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

[Bengt Rutisson]

Hi again,

Mikael just pushed his root scanning changes.

Here's a rebased version of my changes:

http://cr.openjdk.java.net/~brutisso/8027962/webrev.04/

Thanks,
Bengt

On 2015-03-18 14:28, Bengt Rutisson wrote:
>
> 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
>>
>>
>