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

Yumin Qi yumin.qi at oracle.com
Thu May 5 15:38:21 UTC 2011 

On 2011/5/4 23:12, Y. Srinivas Ramakrishna wrote:
> 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.
>
Thanks.
> 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.
>
Good, I will change to check at the end of parsing.

> 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".
>

Yes. The handle at any time only one. As for the limitation of files 
used to rotation, maybe your recommendation is good too, just let the 
users decide how many files they would like to have on their disk.

> 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.
>
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.

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 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.

> 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.
>
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.

> 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 :-)
>
I like this idea.

Thanks
Yumin
> 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
>>>>>>>>>>
>

More information about the hotspot-gc-dev mailing list