RFR (S): 8209843: Optimize oop scan closure closures wrt to reference processing in G1

[Thomas Schatzl]

Hi Kim,

On Sun, 2018-08-26 at 19:38 -0400, Kim Barrett wrote:
> > On Aug 24, 2018, at 10:24 AM, Per Liden <per.liden at oracle.com>
> > wrote:
> > 
> > Hi Thomas,
> > 
> > On 08/24/2018 03:16 PM, Thomas Schatzl wrote:
> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8209843
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8209843/webrev/
> > 
> > About the new assert in iteration.hpp. Isn't it better to have it
> > somewhere in the actual oop_oop_iterate path so that we also catch
> > those cases that were using the constructor? Something like:
> > 
> > --- a/src/hotspot/share/oops/instanceRefKlass.inline.hpp
> > +++ b/src/hotspot/share/oops/instanceRefKlass.inline.hpp
> > @@ -98,12 +98,14 @@
> > 
> > template <typename T, class OopClosureType, class Contains>
> > void InstanceRefKlass::oop_oop_iterate_fields(oop obj,
> > OopClosureType* closure, Contains& contains) {
> > +  assert(closure->ref_discoverer() == NULL, "ReferenceDiscoverer
> > should not be set");
> >   do_referent<T>(obj, closure, contains);
> >   do_discovered<T>(obj, closure, contains);
> > }
> > 
> > template <typename T, class OopClosureType, class Contains>
> > void InstanceRefKlass::oop_oop_iterate_fields_except_referent(oop
> > obj, OopClosureType* closure, Contains& contains) {
> > +  assert(closure->ref_discoverer() == NULL, "ReferenceDiscoverer
> > should not be set");
> >   do_discovered<T>(obj, closure, contains);
> > }
> > 
> > If you do this, I'm thinking you also don't need the asserts you
> > added in g1OopClosures.inline.hpp?
> > 
> > cheers,
> > Per
> 
> I was wondering something similar, but thinking a place for such a
> check might be in the reference_iteration_mode definitions. That is,
> each implementation of that function should verify the discoverer
> state is appropriate for the mode that is going to be returned.
> 
> Though that wouldn't work if there are places that are disabling
> discovery via a NULL discoverer, rather than via disable_discovery().
> But nobody would do that, right? :)

G1 and ZGC use this shortcut to have a NULL discoverer if discovery is
disabled (G1 makes sure both are consistent, ZGC's discoverer does not
have any other way to disable discovery afaict).

There is some code in the serial gc mark sweep compact algorithm to set
the discoverer to NULL, but apparently only as a way to somehow catch
bugs (which eludes me how this should work).

Changing things either way (let G1 never use a NULL discoverer with an
iteration_mode() indicating discovery or change other collectors to use
a NULL discoverer to disable discovery) seems to go beyond what this
RFE is set out to do.

As for asserting, this leaves us with Per's suggestion that I
implemented at

http://cr.openjdk.java.net/~tschatzl/8209843/webrev.1/ (full only,
sorry, but the change is very small)

Thanks,
  Thomas