Request for review: 6941923: RFE: Handling large log files produced by long running Java Applications

[Jesper Wilhelmsson]

Yumin,

I can't see from the flag name of MaxGCLogFileNumbers what it is supposed to 
be used for. A better name could perhaps be: MaxGCLogNumberOfFiles.


In globals.hpp I find the descriptions in the strings a bit weird. Maybe these 
strings are used in some non-standard way, but from a pure C point of view the 
strings will be concatenated into:

"Prevent large gclog file for long app running must use with -Xloggc:file"
"Maximum number of gclog file roration Default rotate in 1 file"
"Default log size 10 Megabytes only used when UseGCLogFileRotation set"

I would rather see something like:

"Prevent large gclog file for long running app. Requires -Xloggc:file"
"Maximum number of gclog files in rotation. Default: 1 file"
"Maximum gclog file size. Default: 10MB. Only used with UseGCLogFileRotation"


In arguments.cpp there are also a few typos:

"-XX:+UseGCLogRotaion must be with -Xloggc:filename in front\n"
could be:
"You must specify -Xloggc:filename before -XX:+UseGCLogRotaion"

"Invalid MaxGCLogFileNumbers(should be > 0): %s\n"
add a space:
"Invalid MaxGCLogFileNumbers (should be > 0): %s\n"


In ostream.hpp there is a comment on line 200:
// current logfile rotation number, from 1 to MaxGCLogFileNumbers-1

Is this correct? According to the implementation in ostream.cpp it should 
probably be from 1 to MaxGCLogFileNumbers since you check for strictly greater 
than in line 403:

if (_cur_file_num > MaxGCLogFileNumbers) _cur_file_num = 1;

I would prefer if you changed it to be from 0 to MaxGCLogFileNumbers-1 though 
since it is more of a standard to start at 0, and it would make the code a bit 
easier to understand. You would also get one less operation in creating the 
filename :-)

Also, Oracle secure coding guidelines disallows the use of sprintf. Use 
snprintf instead. (ostream.cpp lines 365, 406 and 408)

Cheers,
/Jesper


On 04/12/2011 08:41 PM, yumin.qi at oracle.com wrote:
> http://cr.openjdk.java.net/~minqi/6941923/webrev.00/
> <http://cr.openjdk.java.net/%7Eminqi/6941923/webrev.00/>
>
> Summary:
>
> This is a RFE request for having a GC log rotation to prevent Java application
> from over flooding disk with GC output running for long time.
> In the implementation, supply three JVM options
> 1) -XX:+UseGCLogFileRotation must be used with -Xloggc:file
> 2) -XX:MaxGCLogFileNumbers= set limit of rotation file numbers, default to 1,
> maximum set to 1024.
> 3) -XX:GCLogFileSize= can be configured by user how big the file size should
> be. Default to 10M. Minimum set to 512K if given from option is less than 512K.
>
> If MaxGCLogFileNumbers=1, rotating output in same file, i.e write from
> beginning of the file when reach cap of the file; with MaxGCLogFileNumbers > 1
> rotating files sequentially after reach cap in file, file.1, file.2, ...,
> file.<MaxGCLogFileNumbers-1> then back to file, file.1, ...
> Check if rotation needed at safepoint ending.
>
> Tested with multiple GC choices.
>
> Thanks
> Yumin
>
>
>