RFR (7xS): 8175554: Improve G1UpdateRSOrPushRefClosure

[Thomas Schatzl]

Hi all,

  Erik suggested a few more refactorings:

- rename G1ParClosureSuper -> G1ScanClosureBase
- rename a few "oops_in_heap_closure" parameter -> "update_rs_cl"
- move instantiation of closures from oops_into_collection_set_do()
into scan_rem_set()/update_rem_set() methods.

I assume these are the final ones :)

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8175554/webrev.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8175554/webrev.2_to_3/ (diff)
Testing:
jprt

Thanks,
  Thomas

On Thu, 2017-06-22 at 12:44 +0200, Thomas Schatzl wrote:
> 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
> >