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

[Mikael Gerdin]

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

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.

/Mikael

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