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

[Bengt Rutisson]

Hi Eric,

On 2015-03-19 16:29, Eric Caspole wrote:
> I like this fine grain accounting. Looks good.

Thanks for the review!

Bengt

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