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:35:07 UTC 2011 

On 2011/5/5 4:02, Jesper Wilhelmsson wrote:
> Yumin, Ramki,
>
> I don't particularly like the flag name GCLogFileSizeMB. Are there 
> other examples of flags with the MB ending in Hotspot?
>
> As for extending fileStream into rotatingFileStream I can't make up my 
> mind. It would of course be good OO design to do the extension, but 
> isn't the ability to have rotating logs a useful feature in most 
> places where a log file is created? Are there examples within Hotspot 
> where a fileStream is used where it would be conceptually wrong to 
> enable log rotation? If there is, I would vote for the 
> rotatingFileStream, if not I don't have a strong opinion.
I guess it will screw up log with xml output if rotation on. Other logs 
may be OK. So I will have an extended version.

Thanks
Yumin

> /Jesper
>
>
> On 2011-05-05 08: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.
>>
>> 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
>>>>>>>>>>>
>>

More information about the hotspot-gc-dev mailing list