RFR (M): 8074037: Refactor the G1GCPhaseTime logging to make it easier to add new phases
Eric Caspole
eric.caspole at oracle.com
Wed Mar 11 13:43:23 UTC 2015
Hi Bengt,
Sorry I am late getting to this this. My only comment on this change is that it took me a while to figure out "_sub_count" -- it's an optional extra statistic or detail about that phase where it is used. Could _sub_count be named _phase_detail or _thread_work_items or something that better shows what it's used for?
Thanks,
Eric
----- Original Message -----
From: bengt.rutisson at oracle.com
To: thomas.schatzl at oracle.com
Cc: hotspot-gc-dev at openjdk.java.net
Sent: Tuesday, March 10, 2015 7:37:14 AM GMT -05:00 US/Canada Eastern
Subject: Re: RFR (M): 8074037: Refactor the G1GCPhaseTime logging to make it easier to add new phases
Hi Thomas,
On 2015-03-10 11:39, Thomas Schatzl wrote:
> Hi Bengt,
>
> On Tue, 2015-03-10 at 10:48 +0100, Bengt Rutisson wrote:
>> Hi Erik,
> [...]
>> To get this email thread back on track - I'm still looking for reviews
>> for the latest webrev:
>> http://cr.openjdk.java.net/~brutisso/8074037/webrev.03/
>>
> - g1CollectedHeap.cpp:3754, active_workers should be a uint I think to
> match the parameter types.
Good catch.
>
> - minor style issue (feel free to ignore): the change stores the
> G1GCPhaseTimes instances into local variables a few times. That variable
> is sometimes called "timer", "phase_times". Also e.g. G1CLDClosure may
> benefit from introducing a local variable.
Fixed.
>
> - another (feel free to ignore): in the long sum starting with
> g1CollectorPolicy.cpp:1134 the operators are sometimes at the end of the
> preceding line, sometimes before. I would prefer them at the end of the
> preceding line.
Fixed.
> - thanks for removing the PRAGMA's in g1GCPhaseTimes.cpp. :)
Yes, nice to get rid of this.
> Looks good otherwise.
Thanks!
Here's an updated webrev:
http://cr.openjdk.java.net/~brutisso/8074037/webrev.04/
And a diff compared to last time:
http://cr.openjdk.java.net/~brutisso/8074037/webrev.03-04.diff/
Bengt
>
> Thanks,
> Thomas
>
>
>
More information about the hotspot-gc-dev
mailing list