RFR (7xS): 8175554: Improve G1UpdateRSOrPushRefClosure

[Thomas Schatzl]

Hi all,

  after discussion with Erik, I removed one comment, and renamed the
closures to something that resembles their use. Also I had to
reintroduce the G1ParPushRefClosure removed in the initial patch due to
performance regressions.

G1UpdateOrScanRSClosure -> G1ScanObjsDuringUpdateRSClosure
G1ParPushRefClosure -> G1ScanObjsDuringScanRSClosure
G1ParScanClosure -> G1ScanEvacuatedObjClosure

We also found that the mechanism to collect cards that contain
references into the collection set to not lose any remembered set
entries during update RS if there is an evacuation failure is basically
superfluous. Other, existing mechanism make sure that all required
remembered sets are (re-)created in other stages of the GC.

Removal of this code has been decided to be out of scope here.

Webrev:
http://cr.openjdk.java.net/~tschatzl/8175554/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8175554/webrev.2/ (full)
Testing:
jprt, local testing

Thanks,
  Thomas


On Tue, 2017-06-20 at 10:05 +0200, Thomas Schatzl wrote:
> Hi Sangheon, others,
> 
> On Tue, 2017-05-30 at 15:15 -0700, sangheon wrote:
> > 
> > Hi Thomas,
> > 
> > On 05/05/2017 05:13 AM, Thomas Schatzl wrote:
> > > 
> > > 
> > > Hi all,
> > > 
> > >    recent reviews have made changes necessary to parts of the
> > > changeset chain.
> > > 
> > > Here is a list of links to updated webrevs. Since they have
> > > apparently not been reviewed yet, I simply overwrote the old
> > > webrevs.
> > > 
> > > JDK-8177044: Remove _scan_top from HeapRegion
> > > http://cr.openjdk.java.net/~tschatzl/8177044/webrev/
> > > 
> > > JDK-8178148: Log more detailed information about scan rs phase
> > > http://cr.openjdk.java.net/~tschatzl/8178148/webrev/
> > > 
> > > JDK-8175554: Improve G1UpdateRSOrPushRefClosure
> > > http://cr.openjdk.java.net/~tschatzl/8175554/webrev/
> > Looks good to me.
> > I only have minor nits.
> > 
> > ------------------------------------------------------
> > src/share/vm/gc/g1/g1OopClosures.hpp
> >    78   virtual void do_oop(oop* p) { do_oop_nv(p); }
> > Misaligned with above line.
> > 
> > ------------------------------------------------------
> > src/share/vm/gc/g1/g1RemSet.hpp
> >   204                   G1UpdateOrScanRSClosure* push_heap_cl,
> > Rename to reflect new closure name?
> > 
> > ------------------------------------------------------
> > src/share/vm/gc/g1/g1RootProcessor.hpp
> > Copyright update.
> > 
> > ------------------------------------------------------
> > src/share/vm/gc/g1/g1_specialized_oop_closures.hpp
> >    45       f(G1UpdateOrScanRSClosure,_nv)         \
> > Misaligned '\'.
> > 
>   I fixed all this in addition to incorporating ErikD's comments that
> asked for factoring out two parts of the G1ParScanClosure and
> G1UpdateOrScanRSClosure that were equal now.
> 
> I did some performance testing again due to that, and also found that
> the check to filter out non-cross-region references
> in G1UpdateOrScanRSClosure::do_oop_nv() seemed faster, so I also
> reverted it to the old code.
> 
> Also in this change G1UpdateOrScanRSClosure::do_oop_nv() did not
> update
> _has_refs_into_cset as before. Fixed that as well.
> 
> Thanks,
>   Thomas
>