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

[Vladimir Kozlov]

Looks good.

Vladimir K

On 4/25/13 7:50 AM, Vladimir Ivanov wrote:
> Chris,
>
> You were right. Here's updated version:
> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.03/
>
> 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
>>>>>>>>>>>>>
>>>>
>>