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

[Thomas Schatzl]

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.
> >
> > - 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,
  Thomas