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

[Erik Österlund]

Hi Thomas,

Ship it.

Thanks,
/Erik

On 2018-06-28 15:26, Thomas Schatzl wrote:
> Hi Kim and Erik,
>
>    thanks for your comments.
>
> I updated the webrev at
>
> http://cr.openjdk.java.net/~tschatzl/8203848/webrev/
>
> to only contain the cmpxchg change.
>
> Unless somebody objects I will push this in the next few days.
>
> Thanks,
>    Thomas
>
>
> On Thu, 2018-06-28 at 01:00 -0400, Kim Barrett wrote:
>>> On Jun 7, 2018, at 5:47 AM, Thomas Schatzl <thomas.schatzl at oracle.c
>>> om> 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/8203848/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
>> Using AS_NO_KEEPALIVE seems okay to me.
>>