RFR (S): 8012260: ciReplay: Include PID into the name of replay data file

[Vladimir Ivanov]

Chris,

Thanks for review!
Sure, I've updated [1] with your suggestion.

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~vlivanov/8012260/webrev.03/

On 4/25/13 9:08 PM, Christian Thalinger wrote:
>
> On Apr 25, 2013, at 7:50 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>
>> Chris,
>>
>> You were right. Here's updated version:
>> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.03/
>
> Much better.  Thank you.  Perhaps you also want to change your method comments to Doxygen-style ones?
>
> -- Chris
>
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 4/20/13 8:03 AM, Christian Thalinger wrote:
>>>
>>> On Apr 19, 2013, at 3:17 PM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>>>
>>>> Thank you for review, Chris!
>>>>
>>>>> src/os/windows/vm/os_windows.cpp:
>>>>>
>>>>> +   if (buflen > INT_MAX)  n = INT_MAX;
>>>>>
>>>>> Why do we have this check on Windows?
>>>> Since _getcwd requires int on Windows, this is a consequence of size_t -> int conversion.
>>>
>>> Got it.
>>>
>>>>
>>>>> src/share/vm/utilities/ostream.hpp:
>>>>>
>>>>> !   fileStream(FILE* file, bool need_close=false) { _file = file; _need_close = need_close; }
>>>>>
>>>>> Please put spaces before and after "=".
>>>> Sure, will do.
>>>>
>>>>> +        if (pos > 0 &&
>>>>> +            Arguments::copy_expand_pid(default_pattern, strlen(default_pattern),
>>>>> +                                       &buf[pos], buflen-pos)) {
>>>>> +          fd = open(buf, O_RDWR | O_CREAT | O_TRUNC, 0666);
>>>>> +        }
>>>>>
>>>>> Could you factor this code into a method?  It's duplicated three times.
>>>> Yeah, I don't like it either :-) But I don't think that a function call will help here. Maybe factoring parameters may help, but I'm not sure. What do you prefer?
>>>
>>> Putting Arguments::copy_expand_pid and open calls into a method sounds like a win to me.
>>>
>>> -- Chris
>>>
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>>> Otherwise this looks good.
>>>>>
>>>>> -- Chris
>>>>>
>>>>> On Apr 19, 2013, at 10:03 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>>>>>
>>>>>> I need one more "official" review. Any volunteers? :-)
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>> On 4/19/13 7:22 PM, Vladimir Kozlov wrote:
>>>>>>> Looks good.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/19/13 7:17 AM, Vladimir Ivanov wrote:
>>>>>>>> Updated fix:
>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.02/
>>>>>>>>
>>>>>>>> Did some additional cleanup: changed get_current_directory to accept
>>>>>>>> size_t instead of int as a buffer size.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Vladimir Ivanov
>>>>>>>>
>>>>>>>> On 4/17/13 8:30 PM, Vladimir Kozlov wrote:
>>>>>>>>> On 4/17/13 9:03 AM, Vladimir Ivanov wrote:
>>>>>>>>>> I considered it redundant since fdopen returns NULL when a call fails
>>>>>>>>>> [1] and I already check replay_data_file. Was my reasoning wrong?
>>>>>>>>>
>>>>>>>>> My understanding was that you can't pass fd == -1 to fdopen() and I
>>>>>>>>> can't find any docs about such case so you may be right. And I am more
>>>>>>>>> concern about Windows/Cygwin implementation of fdopen.
>>>>>>>>>
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I checked on MacOS and when (fd == -1) it produces the following error
>>>>>>>>>> message:
>>>>>>>>>> # Can't open file to dump replay data. Error: Bad file descriptor
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Vladimir Ivanov
>>>>>>>>>>
>>>>>>>>>> [1] Excerpt from fdopen man page:
>>>>>>>>>> RETURN VALUES
>>>>>>>>>>       Upon successful completion fopen(), fdopen(), and freopen()
>>>>>>>>>> return
>>>>>>>>>> a FILE pointer.  Otherwise, NULL is returned and the global variable
>>>>>>>>>> errno is set to indicate the error.
>>>>>>>>>>
>>>>>>>>>> On 4/17/13 7:54 PM, Vladimir Kozlov wrote:
>>>>>>>>>>> Missing check (fd != -1) before fdopen:
>>>>>>>>>>>
>>>>>>>>>>> 1036       FILE* replay_data_file = fdopen(fd, "w");
>>>>>>>>>>>
>>>>>>>>>>> Vladimir
>>>>>>>>>>>
>>>>>>>>>>> On 4/17/13 8:19 AM, Vladimir Ivanov wrote:
>>>>>>>>>>>> Thanks for feedback, Vladimir!
>>>>>>>>>>>>
>>>>>>>>>>>> Here's updated version:
>>>>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.01
>>>>>>>>>>>>
>>>>>>>>>>>> Best regards,
>>>>>>>>>>>> Vladimir Ivanov
>>>>>>>>>>>>
>>>>>>>>>>>> On 4/17/13 9:22 AM, Vladimir Kozlov wrote:
>>>>>>>>>>>>> Make it product flag since it is ON by default (and it is not
>>>>>>>>>>>>> diagnostic
>>>>>>>>>>>>> but error dump).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also why not reuse code which opens ErrorFile (hs_err*)?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Compilation::compile_only_this_method() allocates fileStream on
>>>>>>>>>>>>> stack:
>>>>>>>>>>>>>
>>>>>>>>>>>>>    fileStream stream(fopen("c1_compile_only", "wt"));
>>>>>>>>>>>>>
>>>>>>>>>>>>> thanks,
>>>>>>>>>>>>> Vladimir
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 4/16/13 5:01 PM, Vladimir Ivanov wrote:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.00
>>>>>>>>>>>>>> 43 lines changed: 16 ins; 21 del; 6 mod;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changed naming scheme for compilation replay data files from
>>>>>>>>>>>>>> 'replay.txt' to 'replay_pid%p.log', where %p is substituted w/
>>>>>>>>>>>>>> pid. It
>>>>>>>>>>>>>> allows to easily correspond replay file with hs_err file and makes
>>>>>>>>>>>>>> overwriting of existing files very unlikely.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also, converted ReplayDataFile & DumpReplayDataOnError flags from
>>>>>>>>>>>>>> develop to diagnostic, since they are useful in product binaries.
>>>>>>>>>>>>>> Did
>>>>>>>>>>>>>> some small cleanups.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> One question still bothers me. There's a memory allocation in
>>>>>>>>>>>>>> VMError::report_and_die() I don't know how to avoid:
>>>>>>>>>>>>>> +         fileStream* replay_data_stream = new
>>>>>>>>>>>>>> (ResourceObj::C_HEAP,
>>>>>>>>>>>>>> mtCompiler) fileStream(replay_data_file);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It's not new, I just moved it up from auxiliary method. Any ideas
>>>>>>>>>>>>>> how to
>>>>>>>>>>>>>> make VMError::report_and_die() allocation-free again? :-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Best regards,
>>>>>>>>>>>>>> Vladimir Ivanov
>>>>>>>>>>>>>>
>>>>>
>>>
>