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

[Bengt Rutisson]

Hi Mikael,

On 11/03/15 15:04, Mikael Gerdin wrote:
> Hi Bengt,
>
> On 2015-03-10 12:32, Bengt Rutisson wrote:
>>
>> 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/
>
> Overall the change looks good, but with the logging the devil is in 
> the details :)

Agreed. I did try to verify that the actual log output has not changed 
at all. :)

>
> I have just a few stylistic issues, I don't need to re-review the 
> change if you want to push it.
>
>
> in g1GCPhaseTimes.cpp
> void G1GCPhaseTimes::note_gc_end() {
>
> You call ->verify() after the calculations instead of before
>
> in g1StringDedup.cpp
> void G1StringDedup::unlink_or_oops_do(BoolObjectClosure* is_alive,
>     OopClosure* keep_alive,
>     bool allow_resize_and_rehash,
>     G1GCPhaseTimes* phase_times) {
>
> The parameters are not lined up properly.

Good. I'll fix this before I push.

Thanks for the review!
Bengt

>
> /Mikael
>
>>
>> And a diff compared to last time:
>> http://cr.openjdk.java.net/~brutisso/8074037/webrev.03-04.diff/
>>
>> Bengt
>>
>>>
>>> Thanks,
>>>    Thomas
>>>
>>>
>>>
>>