RFR: 8144908: Remove apply_to_weak_ref_discovered_field override for UpdateRSOopClosure

[Jon Masamitsu]

Stefan,

At line 41 in "share/vm/oops/instanceRefKlass.inline.hpp"

     38  void 
InstanceRefKlass::oop_oop_iterate_ref_processing_specialized(oop obj, 
OopClosureType* closure,
  Contains& contains) {
     39    T* disc_addr = (T*)java_lang_ref_Reference::discovered_addr(obj);
     40    if (closure->apply_to_weak_ref_discovered_field()) {
     41      Devirtualizer<nv>::do_oop(closure, disc_addr);
     42    }

you're saying that the "next" field of "obj" is
non-null?  Did you try adding a guarantee to that
effect and run your testing (without your change,
of course)?

Jon

On 12/8/2015 5:42 AM, Stefan Johansson wrote:
> Hi,
>
> Please review this change for:
> https://bugs.openjdk.java.net/browse/JDK-8144908
>
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8138888/8144908/hotspot.00/
>
> Background:
> When iterating objects with oop_iterate() there is a special case for 
> InstanceRefKlass, if the closure overrides 
> apply_to_weak_ref_discovered_field() to return true, the closure will 
> be applied to the discovered field, even if the normal checks prevents 
> it. This special treatment is needed in certain situations, but it is 
> not obvious if it should be done when iterating the object. Currently 
> only G1 closures override apply_to_weak_ref_discovered_field() and 
> many of them seem to do it without any reason. My initial plan [1] was 
> to remove all usage of this special casing but after discussions I 
> realized it would be easier to see the effects if doing this one 
> closure at a time.
>
> Summary:
> UpdateRSOopClosure is used to re-build the remembered sets after a G1 
> full GC. This is done after reference discovery and enqueuing (to the 
> pending list) has been completed, so all discovered references have 
> their next-field set. Before the full GC is started all references 
> discovered by the concurrent-mark reference processor is clear to 
> allow them to be discovered during the full GC.
>
> So at the point of applying the UpdateRSOopClosure, no references 
> should have their discovered field set but still be active (next == 
> null). So overriding apply_to_weak_ref_discovered_field to make sure 
> the discovered field is handled doesn't have any effect. Either the 
> discovered and next field are both set and thus gets handled the 
> normal way, or the discovered field is not set and there is no reason 
> to handle it at all.
>
> Testing:
> * RBT
> * GC-test-suite run locally with small heap (to trigger full GC) and 
> verification turned on.
>
> Thanks,
> Stefan
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8138888