RFR: 7164841: Improvements to the GC log file rotation

[Zhengyu Gu]

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
>> 
>