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

[Thomas Schatzl]

Hi,

 ... aaaand I forgot to add the webrevs in the last email for
reference:

http://cr.openjdk.java.net/~tschatzl/8212911/webrev.1_to_2 (diff)
http://cr.openjdk.java.net/~tschatzl/8212911/webrev.2 (full)

Thanks,
  Thomas

On Tue, 2018-10-30 at 11:34 +0100, Thomas Schatzl wrote:
> 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
> 
>