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

[Stefan Karlsson]

On Wed, 10 Aug 2022 14:17:23 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 v1

Changes requested by stefank (Reviewer).

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

> 54: 
> 55:   virtual void do_oop(narrowOop* p) { ShouldNotReachHere(); }
> 56: };

* Keepalive should probably be KeepAlive. (I realize that AS_NO_KEEPALIVE doesn't split the two words, but that one is the outlier in the HotSpot code base)
* Seems unnecessary to cache the heap pointer
* Could this class be moved into the BarrierSetNMethod scope. Or, alternatively give it a more descriptive name stating that this is for nmethods and/or native phantom loads?
 I'm thinking about future name collisions and ODR.
* Could you add a comment stating why you do this dance with AS_NO_KEEPALIVE + keep_alive(oop)

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

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