RFR: 8292077: G1 nmethod entry barriers don't keep oops alive [v4]

[Stefan Karlsson]

On Wed, 10 Aug 2022 15:03:02 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> The intention is that when G1 uses nmethod entry barriers (currently with --enable-preview to support Loom, but soon also with https://bugs.openjdk.org/browse/JDK-8290025), it keeps nmethod oops alive the first time an nmethod becomes on-stack during concurrent marking. The current code fails to do that, even though it intended to. That should be fixed.
>> 
>> There was code from loom to keep the oops alive by performing a phantom load. However, there is a quirk with the access API; if you don't assign the result of the load to something, the load won't be performed, and hence the oop won't be kept alive. I changed the code to use CollectedHeap::keep_alive explicitly instead of relying on side effects of a dummy load.
>> I also found that we assume G1 concurrent marking isn't aborted more than once. But that can happen. I made the assumption more conservative - surely if there are INT_MAX - 1 interrupted concurrent GCs in a row... something is very very wrong.
>
> Erik Österlund has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Thomas comments v2

Thanks. Would you mind a few more tweaks?

src/hotspot/share/gc/shared/barrierSetNMethod.cpp line 67:

> 65:   public:
> 66:     virtual void do_oop(oop* p) {
> 67:       // Loads on nmethod oops are phantom strength. The intend of the load

intend => intent?

src/hotspot/share/gc/shared/barrierSetNMethod.cpp line 71:

> 69:       // concurrent marking. Using the keep alive side effects of a normal
> 70:       // phantom load is less explicit, and doesn't actually do anything
> 71:       // unless the returned value is used as an oop.

I think this is bit vague. Would it be OK if we explicitly spelled out the problem with the LoadOopProxy? Maybe something like this (feel free to tweak the actual text):

// Loads on nmethod oops are phantom strength.
//
// Note: that we could have used NativeAccess<ON_PHANTOM_OOP_REF>::oop_load(p),
// but that would have *required* us to convert the returned LoadOopProxy to an oop,
// or else keep alive load barrier will never be called. It's the LoadOopProxy-to-oop
// conversion that performs the load barriers. This is too subtle, so we instead
// perform an explicit keep alive call.

-------------

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9817