RFR (M): 8074037: Refactor the G1GCPhaseTime logging to make it easier to add new phases

[Bengt Rutisson]

Hi Eric,

Thanks for looking at this!

On 11/03/15 14:43, Eric Caspole wrote:
> 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?

Yes, I was struggling a bit with that name. The reason I picked count 
was that it is currently always some kind of "count" - number of buffers 
processed for example. For that reason I think your suggestion of 
_thread_work_items is better than _phase_detail since that latter does 
not say much about how to use this.

I'll change to _thread_work_items.

Thanks,
Bengt

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