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

[Bengt Rutisson]

Hi everyone,

Here's an updated webrev that fixes the issues that Mikael brought up 
and the renaming that Eric suggested:

http://cr.openjdk.java.net/~brutisso/8074037/webrev.05/

And the diff compared to the last webrev:

http://cr.openjdk.java.net/~brutisso/8074037/webrev.04-05.diff/

Since these were all minor things and renaming I'll go ahead and push 
this. If there are any more comments about the latest webrev I'm happy 
to push a separate cleanup change to fix those comments.

Thanks,
Bengt


On 2015-03-11 15:35, Bengt Rutisson wrote:
>
> 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
>>>
>>>
>>>
>