RFC: Patch for 8203848: Missing remembered set entry in j.l.ref.references after JDK-8203028

[Erik Österlund]

Hi Thomas,

A HeapAccess<AS_NO_KEEPALIVE>::oop_atomic_cmpxchg as you proposed sounds 
good to me.

Thanks,
/Erik

On 2018-06-07 11:47, Thomas Schatzl wrote:
> Hi all,
>
>    I would like to ask for comments on the fix for the issue handled in
> JDK-8203848.
>
> In particular, the problem is that currently the "discovered" field of
> j.l.ref.References is managed completely opaque to the rest of the VM,
> which causes the described error: remembered set entries are missing
> for that reference when doing *concurrent* reference discovery.
>
> There is no problem with liveness of objects referenced by that because
> a) a j.l.ref.Reference object found by reference discovery will be
> automatically kept alive and
> b) no GC in Hotspot at this time evacuates old gen objects during
> marking (and Z does not use the reference processing framework at all),
> so that reference in the "discovered" field will never be outdated.
>
> However in the future, G1 might want to move objects in old gen at any
> time for e.g. defragmentation purposes, and I am a bit unclear about
> Shenandoah tbh :)
>
> I see two solutions for this issue:
> - improve the access modifier so that at least the post-barrier that is
> responsible for adding remembered set entries is invoked on this field.
> E.g. in ReferenceProcessor::add_to_discovered_list_mt(), instead of
>
> oop retest = RawAccess<>::oop_atomic_cmpxchg(next_discovered,
> discovered_addr, oop(NULL));
>
> do a
>
> oop retest =
> HeapAccess<AS_NO_KEEPALIVE>::oop_atomic_cmpxchg(next_discovered,
> discovered_addr, oop(NULL));
>
> Note that I am almost confident that this only makes G1 work as far as
> I understand the access framework; since the previous value is NULL
> when we cmpxchg, G1 can skip the pre-barrier; maybe more is needed for
> Shenandoah, but I hope that Shenandoah devs can chime in here.
>
> I tested this, and with this change the problem does not occur after
> 2000 iterations of the test.
>
> (see the preliminary webrev at http://cr.openjdk.java.net/~tschatzl/820
> 3848/webrev/ ; only the change to referenceProcessor.cpp is relevant
> here).
>
> - the other "solution" is to fix the remembered set verification to
> ignore this field, and try to fix this again in the future when/if G1
> evacuates old regions during marking.
>
> Any comments?
>
> Thanks,
>    Thomas
>