RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Thanks for the review again.

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".
Will change to contain original string used.
> - 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 :-)
If fact the logic is almost same as I used. In extend_file_name function:

pp and pt record the position of %p and %t respectively, if no will be -1.

logic:
no %p and %t: just copy the filename and return;

if (pp > 0 && pt > 0) {   // this is for both showing up
     // this part deal with who is first
     // and copy the AAAA first, pid (or time), BBBB, time (or pid) then 
CCCC
     if ( pp < pt ) { // AAAA%pBBBB%tCCCC
     }
     else { // AAAA%tBBBB%tCCCC
     }
     return filename;   // return point
}

// the following is for only pp > 0 or pt > 0 which  is only one in the 
string. But I find I missed one condition, it should be >=  since they 
can be first char. Same for both appearing in the name.

Will modify  ">' to ">=" and make more tests.

>
> - not sure, but there seems to be a typo in the description of
> dump_loggc_header(): "header" instead of "head", "version" instead of
> "verison"
Will double check spell.
> - 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?
No need, will remove.
> - 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?
Yes, this will change the result file names for all places where 
fileStream is used. This should be OK I think, if we give detail 
documentation of the flags it will affect.
Unless users like using special chars for their file name, this should 
be a nice feature.  I would like to hear more comments on this part.
> - could you update the CR with the changed requirements? I.e. the
> introduction of the %p/%t etc and other limitations?
OK
> Thanks,
> Thomas
>