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

[Yumin Qi]

Thanks again!

Yumin

On 2011/5/20 6:05, Jesper Wilhelmsson wrote:
> Looks good!
> /Jesper
>
>
> On 05/16/2011 07:57 PM, Yumin Qi wrote:
>> Hi, Ramki, Jesper and all
>>
>> Changes has beed made according to previous codereview and 
>> discussion. New
>> webrev available at:
>>
>> http://cr.openjdk.java.net/~minqi/6941923/webrev.05
>>
>> In this version, also a few cleanup changes.
>>
>> The logic now for rotating gc log file is:
>> -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=<num_of_files>
>> -XX:GCLogFileSize=<logsize>
>>
>> UseGCLogFileRotation must be set, if rotation in on depends on other 
>> two flag
>> settings: they must be > 0, either of them is 0 will lead rotation 
>> not set.
>> Added a test case to verify(note it lasts ~5 minutes to check file 
>> rotation).
>>
>> Thanks
>> Yumin
>>
>>
>> On 05/06/11 03: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. 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.
>>>
>>> filename must be provided or rotation will not set.
>>>
>>> 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.
>>>
>>> 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
>>