RFR (M): 6672778: G1 should trim task queues more aggressively during evacuation pauses

[Thomas Schatzl]

Hi all,

On Fri, 2018-04-13 at 14:25 +0200, Stefan Johansson wrote:
> 
> On 2018-04-13 10:35, Thomas Schatzl wrote:
> > Hi Stefan,
> > 
> >    thanks for your review... :)
> > 
> > On Thu, 2018-04-12 at 17:15 +0200, Stefan Johansson wrote:
> > > Hi Thomas,
> > > 
> > > On 2018-04-11 13:46, Thomas Schatzl wrote:
> > > > Hi all,
> > > > 
> > > >     I updated and (hopefully) improved the change a bit after
> > > > some
> > > > more thinking.
> > 
> > [...]
> > > > 
> > > > Webrevs:
> > > > http://cr.openjdk.java.net/~tschatzl/6672778/webrev.0_to_1/
> > > > (diff)
> > > > http://cr.openjdk.java.net/~tschatzl/6672778/webrev.1/ (full)
> > > 
> > > Thanks for this update, I had a glance at the first version and I
> > > think this is indeed simpler. Still have some comments though:
> > > src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> > > 3129 root_processor->evacuate_roots(pss, pss->closures(),
> > > worker_id);
> > > 
> > > Only pass in pss, and get the closures inside evactuate_roots.
> > > -----
> > > src/hotspot/share/gc/g1/g1SharedClosures.hpp
> > > 48 double trim_time_sec() {
> > > 49   return
> > > TicksToTimeHelper::seconds(_oops.trim_ticks_and_reset())
> > > +
> > > 50     TicksToTimeHelper::seconds(_oops_in_cld.trim_ticks_and_res
> > > et()
> > > );
> > > 51 }
> > > 
> > > This could be removed now, right?
> > > -----
> > > 
> > > Apart from this me and Thomas has also spoken a bit offline so
> > > there
> > > might be some additional changes.
> > > 
> > > Thanks,
> > > Stefan
> > 
> >    all done.
> > 
> > http://cr.openjdk.java.net/~tschatzl/6672778/webrev.1_to_2/ (diff)
> > http://cr.openjdk.java.net/~tschatzl/6672778/webrev.2/ (full)
> 
> I like this :) Some additional small comments:
> 
> src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
>   516     _start_time += TicksToTimeHelper::seconds(_trim_time);
> 
> Maybe add a comment to explain when we update _start_time, something
> like:
> // The trim_time should be excluded from this phase, do this by
> // updating the start time.

Fixed.

> -----
> 
> src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp
>   380 class G1GCParPhaseTimesTracker : public CHeapObj<mtGC> {
> 
> Why do you need to change this to be CHeapObj instead of StackObj?

The compiler complains about virtual methods in a StackObj. I did not
want to risk issues due to that. 

> src/hotspot/share/gc/g1/g1ParScanThreadState.hpp
> 
>    64   uint const _stack_drain_upper_threshold;
>    65   uint const _stack_drain_lower_threshold;
> 
> As you know I like "drain" better than "trim" but to be consistent I 
> think we should name these _queue_trim_xxxxx_threshold. Or go the
> other way and name everything "drain" =)

Fixed.

Also fixed a problem with the "-" operator of Tickspan introduced in
all this refactoring. This caused negative times being reported
sometimes in the logs.

http://cr.openjdk.java.net/~tschatzl/6672778/webrev.2_to_3/ (diff)
http://cr.openjdk.java.net/~tschatzl/6672778/webrev.3/ (full)

The change looks really nice now imho, thanks Stefan!

Thanks,
  Thomas