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

[Y. Srinivas Ramakrishna]

Hi Yumin -- First of all my apologies for
the inordinate delay in getting to this...
This mostly looks good, but i have a few general
comments below.

I'd suggest that we don't require any specific ordering of the
parameters. Rather the spec should be completely free form
in terms of order or combination of options. That means
you should defer the consistency checks till after all
options have been parsed, and then do the consistency
checking in the method that does arg consistency checking
or in the method that initializes the gc log file
where these options are used.

Why is there what appears to me to be a somewhat arbitrary
limit of 1024 on the # files in the rotation? Why not
something much larger, like MAX_INT :-) You have a
comment at the spot where you do the bounds-check
that you want to limit the #handles. But that can't
be true because there's at most one file open at
any time: the one being written at that time.
I think the arbitrarily small limit should be
relaxed to something that is more "natural".

I also think (apropos of yr previous communication)
that the file rotation feature should be "continuous"
(in a mathematical sense) with the existing logging,
which would just be an extremal/degenerate case.
Here's a natural way of doing so:
Let the default for NumberOfGCLogFiles be 0, and
interpret that as having a single file without any
numeric suffix. (This is the current case.)
Let the default for GCLogFileSize be 0, and interpret
that, in the spirit of other such options in HotSpot,
as mapping to "infinity", i.e. there is no bound on
the size of the file.
Now even if a user chooses +GCLogFileRotation,
but does not specify anything else, he gets the
current behaviour (in other words, there is no
actual rotation that happens -- internally you can
probably snap the cached value of the boolean back
to false in that case).
If the user chooses #files = 1, then you get
the filename.0, and it grows indefinitely unless
GCLogFileSize is set to a non-zero value. If
GCLogFileSize is a positive value (i think the default
unit should be MB, not bytes: Thus
GCLogFileSize should be renamed GCLogFileSizeMB.
Given the granularity of the rotation, this seems
fine. I doubt that we'll have cases where byte-size
resolution in the spec of this will ever be useful,
so why waste key-strokes) then the single file.0
will be subject to self-rotation.

Finally, can you remind me why you firstly need
the locking in rotate_log(), and secondly
if you do need the locking why you elide it for
the case of the vm thread or a concurrent gc thread?
It would seem to me that these are the only two
types of threads that would ever need to
rotate the log and in each case they are
guaranteed to be the only ones trying too
rotate the log.

Of course there are other threads that may be writing to
the log at that time and holding the lock, yes?
Or is there something that ensures that no thread
is in the midst of writing to the stream when
the VM thread rudely yanks the descriptor from
underneath the suspended thread? Is there an
underlying assumption here about who may be in the
process of writing to that file when these things happen,
or are there locks that prevent such interference?
if so, that should be prominently documented
where the file is being closed.

Finally, a bit of a puritanical nit: Might it not
have been better to have a subclass of filestream,
called a RotatingFileStream which would have the
rotation capability, rather than slipping that capability
directly into fileStream as you've done here?
(I am a bit ambivalent here: perhaps i am being
a bit of an OO design nazi perhaps, so i'll let
others wave me off if this begins to sound too
ideological or dogmatic :-)

Rest looks fine; nice work, and high time we had this
capability. PS: we should hold the CCC spec finalization
until some of the option interpretation issues i brought
up above have been resolved to the satisfaction of
those who have an opinion in the matter.

Sorry for the length and rambling tone of this review note.
-- ramki


On 5/3/2011 9:01 AM, yumin.qi at oracle.com wrote:
> Thanks!
>
> Yumin
> On 5/3/2011 2:04 AM, Jesper Wilhelmsson wrote:
>> Looks good.
>> /Jesper
>>
>> On 05/03/2011 01:56 AM, yumin.qi at oracle.com wrote:
>>> Hi, Jesper and all
>>>
>>> This is third version, removed the flag GCLogFile and related code changes.
>>> Also restore the code inside ostream_init_log.
>>> http://cr.openjdk.java.net/~minqi/6941923/webrev.03
>>>
>>> Thanks
>>> Yumin
>>>
>>> On 5/2/2011 8:33 AM, Jesper Wilhelmsson wrote:
>>>> Yumin,
>>>>
>>>> OK. I think a comment in both places mentioned below would be great since it
>>>> is code that may make sense in the future, but does not right now.
>>>>
>>>> I'm personally not a fan of checking in code that may be needed in the
>>>> future, the risk of code rot is overwhelming. But if it is decided that the
>>>> log file name should be changeable fairly soon, I have no objections in this
>>>> case.
>>>>
>>>> What kind of testing have you done to make sure everything works?
>>>> Is there any new tests for this feature that should be added to some test
>>>> suite?
>>>> /Jesper
>>>>
>>>>
>>>> On 05/02/2011 05:14 PM, yumin.qi at oracle.com wrote:
>>>>> Jesper,
>>>>>
>>>>> On 5/2/2011 7:05 AM, Jesper Wilhelmsson wrote:
>>>>>> Yumin,
>>>>>>
>>>>>> Took a closer look this time and noticed that you introduce a fourth new
>>>>>> flag in addition to the three mentioned in the CR, -XX:GCLogFile. I have to
>>>>>> admit that I like the new name better than the old -Xloggc, but do we really
>>>>>> want to introduce a new flag with identical behavior as the old?
>>>>>>
>>>>> In case we turn this flag into manageable, that is, we supply interface in
>>>>> JVMTI, it can be changed outside. Currently it is only a product flag. The log
>>>>> file name cannot be changed currently but future it should be changeable via
>>>>> JVMTI if debugger tools wants to change log rotation and file name.
>>>>>
>>>>>> If we can deprecate the old flag and remove it in a few releases I would be
>>>>>> happy to endorse the new flag, but I suspect that -Xloggc is quite heavily
>>>>>> used in production environments.
>>>>>>
>>>>>>
>>>>>> I am a bit puzzled by a change in ostream.cpp, in ostream_init_log():
>>>>>>
>>>>>> 807 if (gclog_or_tty != NULL && gclog_or_tty != tty) {
>>>>>> 808 delete gclog_or_tty;
>>>>>> 809 }
>>>>>>
>>>>> Yes, you are right. I add this part to prevent if it is already init'ed ---
>>>>> now it is only initailized once in vm creation. This function is only called
>>>>> once at present. With log name changeable, it can be called multiples.
>>>>>
>>>>> Thanks
>>>>> Yumin
>>>>>> Why is this needed? As far as I can tell gclog_or_tty will never have a
>>>>>> value here, the only assignment to that variable is made on the next line in
>>>>>> the same function and the function will only be called once during
>>>>>> initialization of the jvm. Have you seen cases where this delete is executed?
>>>>>> /Jesper
>>>>>>
>>>>>>
>>>>>> On 04/29/2011 07:15 PM, yumin.qi at oracle.com wrote:
>>>>>>> Jesper,
>>>>>>>
>>>>>>> Thanks. Deleted the comments part, this is the new version:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~minqi/6941923/webrev.02
>>>>>>>
>>>>>>> Thanks
>>>>>>> Yumin
>>>>>>>
>>>>>>> On 4/29/2011 5:25 AM, Jesper Wilhelmsson wrote:
>>>>>>>> Yumin,
>>>>>>>>
>>>>>>>> In ostream.hpp lines 199 - 215 you have added a block of code that is
>>>>>>>> commented out. Personally I don't think we should have code that is
>>>>>>>> commented out in there unless there is a good documentation reason for
>>>>>>>> it. I
>>>>>>>> don't see such a reason here.
>>>>>>>>
>>>>>>>> Looks good otherwise.
>>>>>>>> /Jesper
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 04/28/2011 11:18 PM, yumin.qi at oracle.com wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Need your review on the second time changes:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~minqi/6941923/webrev.01
>>>>>>>>>
>>>>>>>>> Any comments on the revised version? thanks in advance.
>>>>>>>>>
>>>>>>>>> Yumin
>>>>>>>>>