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

[Eric Caspole]

I like this fine grain accounting. Looks good.
Eric


On 3/19/2015 10:29 AM, Bengt Rutisson wrote:
>
> 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
>>>
>>>
>>
>