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

Thu May 5 11:02:23 UTC 2011

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