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

[Kim Barrett]

> 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? :)