G1: Should nmethod entry barriers keep oop constants alive?

[Erik Osterlund]

Hi Richard,

I have *always* thought that having nmethod oops be required to be reachable some other way from the object graph or risk breaking SATB is very fragile. That’s part of the reason why ZGC required nmethod entry barriers from the first release that supported class unloading at all (JDK-12). Meanwhile I have kind of been waiting for a situation in G1 where we would add an oop that ends up not being reachable some other way. I felt pretty certain it would pop up eventually, I just did not know in which shape or form it would appear in. Indeed, it did materialize with @Stable. It can also materialize with final fields that you modify them with reflection, Unsafe or JNI. Are there other holes? I don’t know. And I’d rather not wait to find out.

I think that nmethod oops should stand on their own feet. If the compiler decides to put them in, we shouldn’t rely on other fragile and hard to reason about properties to not break SATB. So whether this bug would have presented itself or not, I have always thought the previous mechanism is too fragile.

Besides, the idea of fixing up the last remaining nmethods that are on-stack when you get to remark, doesn’t really work for loom. Because some of the stacks won’t be mounted on carrier threads. Instead, they could reside only in heap continuations that have been allocated concurrent marking started, and hence will never be visited by a GC. This was indeed the original motivation for adding nmethod entry barriers to G1, before the @Stable stuff appeared.

In other words, I think you should REDO JDK-8297487, and not consider this as a problem only for @Stable.

Thanks,
/Erik

On 30 Jan 2023, at 17:52, Reingruber, Richard <richard.reingruber at sap.com<mailto:richard.reingruber at sap.com>> wrote:

Hi,

the nmethod entry barrier used by G1 keeps oop constants of nmethods alive [1].
As I see it this is only needed if actually constant fields (final or @Stable) are changed after initialization (see JDK-8288970).
Is this correct or are there other cases? [2]

If correct then I'd think it isn't the best solution to the issue. (a) the
keep-alive is almost always redundant (b) it is imprecise and can hinder
unloading of unreachable code because class loaders of inlined or devirtualized
calls are added to the nmethods constants. (c) nmethods with a stale constant
value are kept as long as they are called (d) the stale constant value might be
persisted somewhere which could cause inconsistency.

I'd like to get some feedback on this because I wonder if I should REDO
JDK-8297487 at all (after BACKOUT JDK-8299956).

Thanks, Richard.

[1] Keepalive of nmethod entry barrier used by G1:
    https://github.com/openjdk/jdk/blob/08b24ac7aacaff32577dc07e77ed0961dd804904/src/hotspot/share/gc/shared/barrierSetNMethod.cpp#L100-L103

[2] Could be Loom but I wouldn't see why the keep-alive would be needed there. Otherwise it should be unlikely. After all G1 was working well w/o it.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20230130/635b7d42/attachment.htm>