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

[Jesper Wilhelmsson]

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