RFR (M): 8201172: Parallelize Remset Tracking Update Before Rebuild phase

[Thomas Schatzl]

On Tue, 2018-04-17 at 15:17 -0700, sangheon.kim wrote:
> Hi Thomas,
> 
> On 04/11/2018 04:42 AM, Thomas Schatzl wrote:
> > Hi Stefan,
> > 
> > On Tue, 2018-04-10 at 14:29 +0200, Stefan Johansson wrote:
> > > Hi Thomas,
> > > 
> > > On 2018-04-09 13:16, Thomas Schatzl wrote:
> > > > Hi all,
> > > > 
> > > >     can I have reviews for this straightforward change that
> > > > improves
> > > > the
> > > > "Remset Tracking Update Before Rebuild" phase of the Remark
> > > > pause?
> > > > 
> > > > Performance of this phase has not been a big issue, with ~3000
> > > > regions
> > > > you get around 1ms of time taken, but now it's even faster and
> > > > hopefully if run with 30k regions, there are no nasty suprises
> > > > (or
> > > > not
> > > > as nasty at least).
> > > > 
> > > > Determining the number of threads to use has been done by doing
> > > > a
> > > > few
> > > > measurements and tests and look at which point there does not
> > > > seem
> > > > to
> > > > be any more scaling (i.e. the noise seems higher than the
> > > > improvements), and the pause time much smaller than a
> > > > millisecond.
> > > > 
> > > > CR:
> > > > https://bugs.openjdk.java.net/browse/JDK-8201172
> > > > Webrev:
> > > > http://cr.openjdk.java.net/~tschatzl/8201172/webrev/
> > > 
> > > Looks good, just a few minor things:
> > > src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
> > > 1108     G1UpdateRemSetTrackingBeforeRebuild cl(_g1h, _cm, &_cl);
> > > 1109     _g1h->heap_region_par_iterate_from_worker_offset(&cl,
> > > &_hrclaimer, worker_id);
> > > 1110     Atomic::add(cl.num_selected_for_rebuild(),
> > > &_num_selected_for_rebuild);
> > > 
> > > Using the same names for all closures and and having
> > > num_selected_for_rebuild in both the task and the closure, makes
> > > this code a bit hard to follow. I suggest calling the task member
> > > _total_selected_for_rebuild and also have the getter name match
> > > the
> > > member for the closure.
> > > 
> > > For the closures I'm fine with the members being named _cl, but
> > > it
> > > would help if the instance above were name something else, like
> > > rebuild.
> > 
> > Done.
> > 
> > http://cr.openjdk.java.net/~tschatzl/8201172/webrev.0_to_1 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8201172/webrev.1 (full)
> 
> Webrev.1 looks good.
> I only have minor nits.
> 
> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
> 1011   G1ConcurrentMark* _cm;
> - _cm member seems not really necessary for 
> G1UpdateRemSetTrackingBeforeRebuildTask as it only passes to 
> G1UpdateRemSetTrackingBeforeRebuild and we can get it from
> G1CollectedHeap.

There is no guideline on this, I kind of prefer explicitly passing on
what's needed. I will keep that.

> 
> 1013   uint _total_selected_for_rebuild;
> - volatile?

  I will fix this before pushing.

Thanks,
  Thomas