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

[Y. Srinivas Ramakrishna]

Hi Yumin --

globals.hpp: "roration" should be "rotation"

On 5/6/2011 3:50 PM, yumin.qi at oracle.com wrote:
> Hi, Ramki and all
>
> This is a new webrev for the changes.
> http://cr.openjdk.java.net/~minqi/6941923/webrev.04
>
> In this change, a class rotatingFileStream derived from fileStream used to handle log rotation.

Thanks for making that change. Looks good (modulo a few comments below re locking.)

> UseGCLogFileRotation is a must for doing gc log rotation, and it depends on other two flag setting
> for the rotation really to happen:
>
> To enable gc log rotation,
> -Xloggc:<filename> -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=<num_of_files>
> -XX:GCLogFileSize=<num_of_size>
>
> where num_of_files > 0 and num_of_size > 0
> Either one of above is 0 will lead to not having GC log rotation, this is the default as no changes.
> For NumberOfGCLogFiles=1, the log file name will be <filename>.0 and log output rotation happens in
> one file. For NumberOfGCLogFiles > 1, happens in multiple files.

Seems reasonable and I am OK with that (the preference of defaults
becomes a bit subjective, and from an aesthetic standpoint I might
personally have preferred my previous suggestion, but won't mind
what you have). I'll let you work with the CCC to get these
to be something reasonable.

(More on consistency checking further below.)

>
> filename must be provided or rotation will not set.

Good.

>
> I added another flag, MinGCLogFileSize, this is for test purpose, to make the test case can run in a
> short period to verify the code changes. Default value is 32MB, if GCLogFileSize set to less than
> this number, it will be set to MinGCLogFileSize.

I didn't understand this.
Why do you need a minimum size for testing? Smaller sizes would
allow the test verification to run faster, it would seem to me.

It might be reasonable to have a minimum size, but there is nothing inherent
that forces that upon us, does it? Then why have it? Perhaps for safety,
so you do not run the system out of inodes? And if you had a minimum,
you should probably allow GCLogFileSize=0
to also be overridden in the same way. Also, while 32MB seems a reasonable
minimum to me, I really have no idea how one might choose a reasonable default
for this, especially if JavaSE would be used across a range of
device sizes. As I said before, I didn't really see the need for this
since GCLogFileSize could have used a default size which kicks
in if a specific size is not specified. But perhaps it is reasonable
to say files should not be less than a certain size, so that the
file system does not get overstretched having to deal with a very very
large number of files in a single directory?
OTOH, if you introduced this solely for the purpose of testing,
then it should not be a product flag.

On consistency checking: as i noted before, the flag parsing should
not do the consistency checking. Rather, you should move all of
that consistency checking code into the method Arguments::check_vm_args_consistency()
-- if you want to encapsulate stuff related to rtotation into one,
create a new method that's called from there, much like
Arguments::check_gc_consistency() is. If you decide to override
a user-specified setting because of inconsistency, make sure to
issue a warning in those cases.

Finally, on locking: did you try using ttyLocker() instead of
doing the raw locking you are doing here? I notice a number of
cutouts for the locking path in hold(): we'd need to do a careful audit
to make sure this does not leave open the possibility of
leakage of the file descriptor outside of the lock. I did
not have the time to do such an audit, but it would be a good
idea to do so. (Have all kinds of logging -- every conceivable kind
of GC logging enabled and then run with rotation at the smallest
possible size, say just a handful of bytes, enabled -- in order to
stress test this.)

-- ramki


>
> As mentioned, a test case added to verify the result.
>
> Thanks
> Yumin
>
> On 5/5/2011 9:35 AM, Y. S. Ramakrishna wrote:
>>
>> Hi Yumin --
>>
>> On 05/05/11 08:38, Yumin Qi wrote:
>> ...
>>> So, default behavior not changed if GCLogFileSize=0 and NumberOfGCLogFiles=0.
>>> If #file > 0, but GCLogFileSize=0, it is still confused since we use file.0 and output to it at
>>> no limit. The #file seems only make sense for 1.
>>
>> Well, I was using a limit ordinal analogy here. When you reach the
>> first limit ordinal, \omega, you will flip to the next file.
>> Of course in real life we won't. But it's OK to do what you
>> suggest below. It just entails extra args checking, which is fine too.
>>
>>>
>>> I would like if #file > 0 and GCLogFileSize=0, we still keep current behavior - no rotation.
>>> (GCLogFileSize default is 0, no limit as you said).
>>
>> That's fine too.
>>
>>>
>>> That is, if rotation stands, UseGCLogRotation ,must be true.
>>> 1) If GCLogFileSize is default (0), no rotation; UseGCLogFileRotation is false
>>> 2) If #file is default (0), no rotation. UseGCLogFileRotation is false.
>>
>> OK; i am sure you will need to straighten this out in the
>> CCC spec and spell out the restrictions if any on
>> the allowed combinations etc.
>>
>>> This is why I need your review here. The rotation only done at STW, so seems the lock is not
>>> necessary here. Remove the locking code, will put comments to indicating this function only
>>> called at safepoint and add assert at safepoint.
>>
>> It's fine to do the locking. I have no objection to the locking;
>> my question was why the locking was elided for those two cases.
>> It would seem to me that you'd need some way to make sure that
>> the stream is not under active use when the rotation occurs,
>> and the lock would be a fine way to ensure that the stream is
>> never accessed except under protection of the lock. I can see
>> that the rotation is done at a STW pause when no mutator is
>> running. It turns out today that no daemons run at that time
>> either, but it is conceivable that some may in the future
>> and have access to or attempt tp access that stream/log.
>> We should make the code robust in the face of future such
>> evolution, by at least putting in enough controls, and
>> the lock would seem to me to be a fine mechanism to do so.
>> (After auditing the code to make sure that the handle is
>> not accessible outside the lock.)
>>
>> -- ramki