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

[Thomas Schatzl]

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.
>