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

[yumin.qi at oracle.com]

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