RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Thanks for the review.

Yumin
On 9/10/2013 12:37 PM, Zhengyu Gu wrote:
> Good to me.
>
> -Zhengyu
>
> On Sep 9, 2013, at 3:11 PM, Yumin Qi wrote:
>
>> Hi, Thomas and all
>>
>>   This is a new version,
>> http://cr.openjdk.java.net/~minqi/7164841/webrev04/ <http://cr.openjdk.java.net/%7Eminqi/7164841/webrev04/>
>>
>>   There is an existing method, make_log_name which converts %p to <1234>, so I decided to modify it to include converting %t to YYYY-MM-DD_HH-MM-SS. The function extend_file_name is not needed then.  The name limitation is only for file name not path, in case user has special characters used in their path name.
>>
>>   rotatingFileStream is renamed to gcLogFileStream, and handle both logging (normal) and rotating work.
>>
>>   A problem observed when user gives an wrong path, say, -Xloggc:/temp/temp/test.log, /temp/temp does not exist, in this case, I only give a warning and not exit from VM, should app exit VM in such case? After warning, app still can run but no log.
>>
>>   Thanks
>>   Yumin
>>
>>
>>
>>
>> On 9/6/2013 3:27 AM, Thomas Schatzl wrote:
>>> hi,
>>>
>>> On Wed, 2013-09-04 at 19:35 -0700, Yumin Qi wrote:
>>>> Hi, Lois and all
>>>>
>>>>    This is fourth version after suggestions from Lois,
>>>>    URL: http://cr.openjdk.java.net/~minqi/7164841/webrev03
>>>>
>>>>    Add a function dump_loggc_header to fileStream to dump vm version
>>>> etc information as reusable function.
>>>>    Also some minor format and comments changes.
>>> Some notes (I hope this is the most recent patch, the thread is really
>>> long now):
>>>
>>> - The error string in Arguments::parse_each_vm_init_arg() contains a
>>> typo:
>>>
>>> 2857        jio_fprintf(defaultStream::output_stream(),
>>> 2858                   "Filename can only cantain [A-Z][a-z][0-9]-_.%%[p|t]\n"
>>> 2859                   "%%p and %%t can only show up once\n" );
>>>
>>>
>>> "contain" instead of "cantain". Maybe add full stops after each sentence
>>> (and quote the list of allowed characters). It may also be useful to
>>> show the user the bad string again.
>>>
>>> I.e. something like:
>>>
>>> Invalid file name for use with -Xloggc: Filename can only contain the
>>> characters .... but has been "string".
>>>
>>> - insertion of the time and pid strings seem to be coded somewhat
>>> complicated. I.e. it looks like
>>>
>>> if (both time and pid strings in file name) {
>>>    insert time string
>>>    insert pid string
>>> } else if (only time string in file name) {
>>>    insert time string
>>> } else if (only pid string in file name) {
>>>    insert pid string
>>> }
>>>
>>> but wouldn't it be simpler to do that in one go? I.e.
>>>
>>> given a file_name string of the format AAAA%aBBBB%bCCCC (may degenerate
>>> into BBBBB%bCCCC), you could copy first everything up until %a (the
>>> first marker), insert the first marker string, add BBBB, add the second
>>> marker string, and then add CCCC.
>>>
>>> I.e. something *like* this (not tested, may crash etc).
>>>
>>> // note: we already made sure that there is something to replace here,
>>> i.e. at least one of pp/pt is >= 0
>>>
>>> if (pp < pt) {
>>> pos_first = pp; // <--- instead of pp something like pos_pid would read
>>> better btw
>>> str_first = pidtext;
>>> pos_second = pt;
>>> str_second = timestr;
>>> } else {
>>> pos_first = pt;
>>> str_first = timestr;
>>> pos_second = pp;
>>> str_second = pidtext;
>>> }
>>>
>>> // if one of the replacement string has not been set (i.e. pos_first is
>>> -1), it is the first one now
>>>
>>> (note: why does timestr has a "str" suffix but pid a "text" one?)
>>>
>>> size_t copied_file_name_chars = 0; // number of chars copied so far from
>>> file_name
>>> if (pos_first > 0) { // if there is a first marker
>>>    size_t length_to_copy = pos_first + 1;
>>>    strncat(extended_name, file_name, length_to_copy); // copy AAAA part.
>>>    strcat(extended_name, str_first); // add first marker string
>>>    copied_file_name_chars += length_to_copy + 2; // remember that we
>>> already copied the AAAA part. 2 is the length of the replacement
>>> markers; maybe make a constant out of it.
>>> }
>>> size_t length_to_copy = pos_second - copied_file_name_chars + 1; // how
>>> long is the BBBB part?
>>> strncat(extended_name, file_name + copied_file_name_chars,
>>> length_to_copy); // add the BBBB part
>>> strcat(extended_name, str_second); // add the second marker
>>> copied_file_name_chars += length_to_copy + 2; // remember how much we
>>> copied from the file_name string
>>> strcat(extended_name, file_name + copied_file_name_chars); // copy CCCC
>>> part
>>>
>>> Untested :-)
>>>
>>> - not sure, but there seems to be a typo in the description of
>>> dump_loggc_header(): "header" instead of "head", "version" instead of
>>> "verison"
>>>
>>> - in fileStream::dump_loggc_header() there is a missing space after the
>>> "if"
>>>
>>> - why do we need an explicit "this->" use in
>>> fileStream::dump_loggc_header() to call the print member methods?
>>>
>>> - the change does the file name replacement for *all* file streams ever
>>> created by the VM. This seems unexpected, and is not documented at all
>>> in the ostream.hpp file. I would prefer if it would only be done for the
>>> logging file.
>>> I.e. it is probably best to subclass fileStream with a gclogfilestream
>>> or whatever and use that for the log file. But maybe that has already
>>> been discussed - I haven't found anything about that though?
>>>
>>> - could you update the CR with the changed requirements? I.e. the
>>> introduction of the %p/%t etc and other limitations?
>>>
>>> Thanks,
>>> Thomas
>>>