Hotspot-gc-dev

Popular Threads From Hotspot-gc-dev:

What influences young generation pause times?

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

Heap OOM for no apparent reason?

G1GC Full GCs

Hg: jdk7/hotspot-gc/hotspot: 2 new changesets

List Statistics

Total Threads: 1110
Total Posts: 987

Phrases Used to Find This Thread

Page 1 of 1

14-04-2011 02:12 AM

Hotspot-gc-dev member admin is online now

User

28-04-2011 10:18 PM

Hotspot-gc-dev member admin is online now

User

29-04-2011 01:25 PM

Hotspot-gc-dev member admin is online now

User

29-04-2011 06:15 PM

Hotspot-gc-dev member admin is online now

User

02-05-2011 03:05 PM

Hotspot-gc-dev member admin is online now

User

02-05-2011 04:14 PM

Hotspot-gc-dev member admin is online now

User

02-05-2011 04:33 PM

Hotspot-gc-dev member admin is online now

User

02-05-2011 05:50 PM

Hotspot-gc-dev member admin is online now

User

03-05-2011 12:56 AM

Hotspot-gc-dev member admin is online now

User

#10

03-05-2011 10:04 AM

Hotspot-gc-dev member admin is online now

User

#11

03-05-2011 05:01 PM

Hotspot-gc-dev member admin is online now

User

#12

05-05-2011 07:12 AM

Hotspot-gc-dev member admin is online now

User

#13

05-05-2011 12:02 PM

Hotspot-gc-dev member admin is online now

User

#14

05-05-2011 04:35 PM

Hotspot-gc-dev member admin is online now

User

#15

05-05-2011 04:38 PM

Hotspot-gc-dev member admin is online now

User

Hi,

In the case of Fedora14, syslog and any other log files are rotated by "logrotate"
tool. logrotate is invoked by "cron.daily" .

I and co-worker want to match GC log rotation to the timing of other logs.
In the case of Linux and UNIX, it can be achieved by using signal (e.g. SIGHUP).
However, if we use signal, its implementation is platform-dependent.
So I suggested using AttachListener. If we make GC log rotation invoker like
jinfo, jmap, etc... , I'm sure that it's platform-independent.

I understood that my suggestion should be a new RFE.
After that GC log rotation patch merged HotSpot repository, I want to make the patch
to achieve this RFE.

Thanks,

Yasumasa

(2011/04/14 2:54), wrote:
> Thanks for pointing this out, only manageable flags can be changed from outside.
> If change UseGCLogFileRotation to manageable, means it can be changed during java app running. So it can be switched on and off if -Xloggc: gives a file name. As you pointed out, if this flag is manageable, it should go file a CCC request since it can be changed by outside tools. I agree with you that no need to do so now.
>
> Yasumasa's suggestion is add one more cmd in AttachOperationFunctionInfo [] as "gclogrotate", if UseGCLogFileRotation is on, this cmd will call gclog_or_tty->rotate_log(), else print out err msg.
>
> I am not sure if I understand the suggestion well. If this way, it has no harm to the existing implementation since the log is already in rotation. It also has no effect to the logging mechanism.
>
> What I thought is that what if outsider changes UseGCLogFileRotation when java is running, now it is not a problem --- not an external flag.
>
> I will keep the current implementation.
>
> Thanks
> Yumin
>
>
> On 4/13/2011 10:30 AM, Y. S. Ramakrishna wrote:
>> But I agree with Jon that that would be a further new RFE,
>> rather than doing all of that at once.
>>
>> Also, since the flags seem to be designed as a supported public interface,
>> a CCC (or appropriate) review request should be filed, so that
>> any changes stemming from that review are addressed before
>> this is pushed...
>>
>> -- ramki
>>
>> On 04/13/11 10:24, Y. S. Ramakrishna wrote:
>>> A possibility to pursue:
>>>
>>> You could make the appropriate flags manageable, and re-sample/re-snapshot
>>> into private variable (and when appropriate re-initialize the streams)
>>> at each safepoint where you now just check for log rotation.
>>>
>>> -- ramki
>>>
>>> On 04/13/11 09:54, wrote:
>>>> Current implementation will not work with outside tool, ostream_init called once in JVM starting and flags will not be changed rest of running supposedly. Now need to think of what if flags changed by outside tool. Need reconsideration of changing flags from outside tool.
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>> On 4/13/2011 8:47 AM, Jon Masamitsu wrote:
>>>>> Yasumasa ,
>>>>>
>>>>> Sounds like this is a request for an enhancement of the
>>>>> feature that Yumin has implemented. Or rather maybe
>>>>> just a way of turning it on? Or is it something more basic
>>>>> to the feature?
>>>>>
>>>>> Jon
>>>>>
>>>>> On 4/12/2011 5:55 PM, Yasumasa Suenaga wrote:
>>>>>> Hi Yumin,
>>>>>>
>>>>>> I would like to rotate GC log triggered by the outside.
>>>>>> I think that it can be implemented using AttachLister (AttachOperationFunctionInfo [])
>>>>>> and invoker tool.
>>>>>>
>>>>>> Could you examine it?
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>> (2011/04/13 3:41), wrote:
>>>>>>> http://cr.openjdk.java.net/~minqi/6941923/webrev.00/
>>>>>>>
>>>>>>> Summary:
>>>>>>>
>>>>>>> This is a RFE request for having a GC log rotation to prevent Java application from over flooding disk with GC output running for long time.
>>>>>>> In the implementation, supply three JVM options
>>>>>>> 1) -XX:+UseGCLogFileRotation must be used with -Xloggc:file
>>>>>>> 2) -XX:MaxGCLogFileNumbers= set limit of rotation file numbers, default to 1, maximum set to 1024.
>>>>>>> 3) -XX:GCLogFileSize= can be configured by user how big the file size should be. Default to 10M. Minimum set to 512K if given from option is less than 512K.
>>>>>>>
>>>>>>> If MaxGCLogFileNumbers=1, rotating output in same file, i.e write from beginning of the file when reach cap of the file; with MaxGCLogFileNumbers > 1 rotating files sequentially after reach cap in file, file.1, file.2, ..., file. then back to file, file.1, ...
>>>>>>> Check if rotation needed at safepoint ending.
>>>>>>>
>>>>>>> Tested with multiple GC choices.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Yumin
>>>>>>>
>>>>>>>
>>>>>>>
>>>

--
日本電信電話株式会社研究企画部門 OSS センタ
応用技術ユニット　Webグループ

末永　恭正（すえなが　やすまさ）

TEL: 03-5860-5105 (直通 5069)
E-mail:
)
Based on first round review comments, new webrev:

http://cr.openjdk.java.net/~minqi/6941923/webrev.01

One more flag added, GCLogFile which can be used as -Xloggc::
-XX:GCLogFile=

Deleted class gclogStream, modify existing class fileStream instead to
have the functions of the former.

Thanks
Yumin

On 4/12/2011 11:41 AM, wrote:
> http://cr.openjdk.java.net/~minqi/6941923/webrev.00/
>
>
> Summary:
>
> This is a RFE request for having a GC log rotation to prevent Java
> application from over flooding disk with GC output running for long time.
> In the implementation, supply three JVM options
> 1) -XX:+UseGCLogFileRotation must be used with -Xloggc:file
> 2) -XX:MaxGCLogFileNumbers= set limit of rotation file numbers,
> default to 1, maximum set to 1024.
> 3) -XX:GCLogFileSize= can be configured by user how big the file size
> should be. Default to 10M. Minimum set to 512K if given from option is
> less than 512K.
>
> If MaxGCLogFileNumbers=1, rotating output in same file, i.e write from
> beginning of the file when reach cap of the file; with
> MaxGCLogFileNumbers > 1 rotating files sequentially after reach cap in
> file, file.1, file.2, ..., file. then back to
> file, file.1, ...
> Check if rotation needed at safepoint ending.
>
> Tested with multiple GC choices.
>
> Thanks
> Yumin
>
>
>

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, 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
>
)
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, 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
>>
)
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?

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 }

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, 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, 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
>>>
)
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, 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, 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
>>>>
)
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, 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, 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, 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
>>>>>
)
Jesper,

After rethink the code changes, I decided to take your suggestion to
remove the newly added flag -XX:GCLogFile (I like this name much than
the existing -Xloggc) and the code inside ostream_init_log(). If we in
future add those interface for JVMTI, they should be in new changes but
here. Thanks for your review. Will send out a new webrev to you.

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, 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, 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, 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
>>>>>>
)
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, 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, 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, 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
>>>>>>
)
Looks good.
/Jesper

On 05/03/2011 01:56 AM, 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, 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 de**** 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, 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, 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
>>>>>>>
)

Thanks!

Yumin
On 5/3/2011 2:04 AM, Jesper Wilhelmsson wrote:
> Looks good.
> /Jesper
>
> On 05/03/2011 01:56 AM, 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, 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, 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, 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
>>>>>>>>
)
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, wrote:
> Thanks!
>
> Yumin
> On 5/3/2011 2:04 AM, Jesper Wilhelmsson wrote:
>> Looks good.
>> /Jesper
>>
>> On 05/03/2011 01:56 AM, 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, 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, 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, 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
>>>>>>>>>

)
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, wrote:
>> Thanks!
>>
>> Yumin
>> On 5/3/2011 2:04 AM, Jesper Wilhelmsson wrote:
>>> Looks good.
>>> /Jesper
>>>
>>> On 05/03/2011 01:56 AM, 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, 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, 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, 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
>>>>>>>>>>
>
)
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, wrote:
>>> Thanks!
>>>
>>> Yumin
>>> On 5/3/2011 2:04 AM, Jesper Wilhelmsson wrote:
>>>> Looks good.
>>>> /Jesper
>>>>
>>>> On 05/03/2011 01:56 AM, 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, 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, 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, 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
>>>>>>>>>>>
>>

)
On 2011/5/4 23: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.
>
Thanks.
> 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.
>
Good, I will change to check at the end of parsing.

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

Yes. The handle at any time only one. As for the limitation of files
used to rotation, maybe your recommendation is good too, just let the
users decide how many files they would like to have on their disk.

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

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

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

> 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 :-)
>
I like this idea.

Thanks
Yumin
> 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, wrote:
>> Thanks!
>>
>> Yumin
>> On 5/3/2011 2:04 AM, Jesper Wilhelmsson wrote:
>>> Looks good.
>>> /Jesper
>>>
>>> On 05/03/2011 01:56 AM, 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, 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, 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, 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
>>>>>>>>>>
>

)

#16

05-05-2011 05:10 PM

Hotspot-gc-dev member admin is online now

User

#17

05-05-2011 05:35 PM

Hotspot-gc-dev member admin is online now

User

#18

06-05-2011 11:50 PM

Hotspot-gc-dev member admin is online now

User

#19

09-05-2011 05:21 PM

Hotspot-gc-dev member admin is online now

User

#20

09-05-2011 05:41 PM

Hotspot-gc-dev member admin is online now

User

Page 1 of 1

NewsArc Lists | Culture Pages | Computing Archive | Media-Pages

Link to this page on your blog or website by copying the HTML code below and pasting it into your site:

Next Thread »