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

[Per Liden]

Hi Thomas,

On 08/24/2018 03:16 PM, Thomas Schatzl wrote:
> Hi all,
> 
>    can I have reviews for this small change that micro-optimizes
> reference discovery for G1?
> 
> So many closures we use actually do not set a reference processor
> because they do not need to do reference processing, but still set a
> reference processing mode to try exactly that.
> 
> The impact is that for every instanceKlass instance, we check whether
> the reference processor is NULL needlessly (it always is).
> 
> This is unnecessary, and this change fixes the reference processing
> mode for these closures.
> 
> Additionally I added some asserts to verify that
> - for these closures we never set a reference processor (is always
> NULL)
> - if we set a non-NULL reference processor, the reference iteration
> mode actually does some kind of discovery - it does not make sense to
> set a non-NULL reference processor and the reference iteration mode
> does not actually indicate any kind of reference processing.
> Unfortunately we can't do this check in the constructor (querying the
> reference processing mode is a virtual method), nor can we check that
> if we set a NULL reference processor, that we do not try discovery as
> some closures are used both with a NULL and non-NULL reference
> processor. This could be done, but would require some rework for most
> likely no (performance) gain anyway.
> 
> I did not do perf testing, as I do not really expect any difference.
> 
> This additional specialization could be picked up if/when we do other
> specialization of that closure (e.g. for scanning evacuated objects
> that are known to be in young or old).
> 
> 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

> Testing:
> hs-tier1-4,jdk-tier1-3
> 
> Thanks,
>    Thomas
>