RFR (S): 8212911: Unify and micro-optimize handling of non-in-collection set references in oop closures

[Thomas Schatzl]

Hi Kim,

On Mon, 2018-10-29 at 20:54 -0400, Kim Barrett wrote:
> > On Oct 25, 2018, at 5:45 AM, Thomas Schatzl <
> > thomas.schatzl at oracle.com> wrote:
> > 
> > Hi all,
> > 
> > On Wed, 2018-10-24 at 13:36 +0200, Thomas Schatzl wrote:
> > > Hi all,
> > > 
> > >  can I have reviews for this small change that does some minor
> > > cleanup and micro-optimization of our main g1 stw/concurrent gc
> > > closures?
> > > 
> > > [...]
> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8212911
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8212911/webrev/
> > > Testing:
> > > hs-tier1-3, several hs-tier1-5 runs with other changes, benchmark
> > > suite etc.
> > 
> >  Stefan pointed out that _from_in_young is only used in
> > G1ScanEvacuatedObjClosure any more, so it could be moved from
> > G1ScanClosureBase.
> > 
> > Also we brainstormed a bit about the name of _from_in_young, and
> > agreed on changing this to _scanning_in_young.
> > 
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8212911/webrev.1 (full)
> > http://cr.openjdk.java.net/~tschatzl/8212911/webrev.0_to_1 (diff)
> > 
> > Thanks,
> >  Thomas
> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1OopClosures.inline.hpp
>  178   } else {
>  179     if (HeapRegion::is_in_same_region(p, obj)) {
>  180       return;
>  181     }
> 
> I think this would be easier to read if it were written as
> 
>  178   } else if (!HeapRegion::is_in_same_region(p, obj)) {
> 
> There are similar snippets at line 86 and line 200.  I don’t need
> a new review if you want to make these changes.

Fixed.

> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1OopClosures.hpp
>   91     G1ScanClosureBase(g1h, par_scan_state),
> _scanning_in_young(false) { }
> 
> I kind of dislike the initialization of _scanning_in_young to false.

"False" is the safe option, potentially enqueuing a reference that is
not needed to enqueue (if you forget).

> But that seems consistent with previous usage of _from.  For catching
> bugs, a kind of tri-state and an RAII-style setter (from "unset" to
> true or false then back) might be better.  If you like that idea but
> don't want to do it right now, feel free to file an RFE.  (The reset
> back to "unset" could be DEBUG_ONLY, along with all of the relevant
> checking being in asserts.)

I filed JDK-8213142 for that, with an RFR coming soon.

Thanks,
  Thomas