RFR: 7164841: Improvements to the GC log file rotation

[Thomas Schatzl]

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