RFR (M): 8027962: Per-phase timing measurements for strong roots processing

[Bengt Rutisson]

Hi Thomas and Mikael,

Thanks for looking at this!

On 2015-03-18 14:06, Thomas Schatzl wrote:
> Hi Mikael,
>
> On Wed, 2015-03-18 at 14:05 +0100, Mikael Gerdin wrote:
>> Thomas,
>>
>> On 2015-03-18 13:39, Thomas Schatzl wrote:
>>> Hi Bengt,
>>>
>>>     thanks for taking this over...
>>>
>>> On Wed, 2015-03-18 at 12:45 +0100, Bengt Rutisson wrote:
>>>> Hi everyone,
>>>>
>>>> When this was first sent out for review I suggested a couple of cleanup
>>>> changes before doing this change. One of the cleanups has been pushed:
>>>>
>>>> 8074037: Refactor the G1GCPhaseTime logging to make it easier to add new
>>>> phases
>>>> https://bugs.openjdk.java.net/browse/JDK-8074037
>>>>
>>>> The other one is out for review and is close to being pushed:
>>>>
>>>> 8075210: Refactor strong root processing in order to allow G1 to evolve
>>>> separately from GenCollectedHeap
>>>> https://bugs.openjdk.java.net/browse/JDK-8075210
>>>>
>>>> Based on the above patches, here's an updated webrev to fix 8027962:
>>>>
>>>> http://cr.openjdk.java.net/~brutisso/8027962/webrev.02
>>>>
>>>> Could I have some reviews for this change?
>>> - G1GCPhaseTimes::note_gc_start does not use the mark_in_progress
>>> parameter any more and can be removed.

Good catch. I removed the mark_in_progress parameter.

Here's an updated webrev:
http://cr.openjdk.java.net/~brutisso/8027962/webrev.03/

And an incremetal diff compared to the last one:
http://cr.openjdk.java.net/~brutisso/8027962/webrev.02-03.diff/

>>>
>>> - this is more some style issue: since we moved all G1 specific root
>>> processing to G1RootProcessor, we actually do not need to pass around
>>> the phase_times parameter any more.
>>> We already assume that the G1GCPhaseTimes instance is global in the
>>> entire G1 code, so code in G1RootProcessor could also just access it via
>>> the global.
>>> Just store it in a local variable to save typing at the beginning of the
>>> method. Actually G1RootProcessor::evacuate_roots() does exactly that
>>> unlike process_vm_roots() and process_java_roots().
>>> It is up to you to decide.
>> The problem is that verification (VerifyBefore/AfterGC) also uses
>> G1RootProcessor, but does not want the phase times reported (at least
>> not in the global phase times instance).
>> So in the end we need some way to determine if we should report the time
>> or not for each root step.
> Okay, I overlooked that. Forget this suggestion then.

Thanks for sorting this out.

Bengt

>
> Thanks,
>    Thomas
>
>