RFR: 8290892: C2: Intrinsify Reference.reachabilityFence [v27]

Mon Dec 8 10:38:29 UTC 2025

On Sat, 15 Nov 2025 02:28:55 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> This PR introduces C2 support for `Reference.reachabilityFence()`.
>> 
>> After [JDK-8199462](https://bugs.openjdk.org/browse/JDK-8199462) went in, it was discovered that C2 may break the invariant the fix relied upon [1]. So, this is an attempt to introduce proper support for `Reference.reachabilityFence()` in C2. C1 is left intact for now, because there are no signs yet it is affected.
>> 
>> `Reference.reachabilityFence()` can be used in performance critical code, so the primary goal for C2 is to reduce its runtime overhead as much as possible. The ultimate goal is to ensure liveness information is attached to interfering safepoints, but it takes multiple steps to properly propagate the information through compilation pipeline without negatively affecting generated code quality.
>> 
>> Also, I don't consider this fix as complete. It does fix the reported problem, but it doesn't provide any strong guarantees yet. In particular, since `ReachabilityFence` is CFG-only node, nothing explicitly forbids memory operations to float past `Reference.reachabilityFence()` and potentially reaching some other safepoints current analysis treats as non-interfering. Representing `ReachabilityFence` as memory barrier (e.g., `MemBarCPUOrder`) would solve the issue, but performance costs are prohibitively high. Alternatively, the optimization proposed in this PR can be improved to conservatively extend referent's live range beyond `ReachabilityFence` nodes associated with it. It would meet performance criteria, but I prefer to implement it as a followup fix.
>> 
>> Another known issue relates to reachability fences on constant oops. If such constant is GCed (most likely, due to a bug in Java code), similar reachability issues may arise. For now, RFs on constants are treated as no-ops, but there's a diagnostic flag `PreserveReachabilityFencesOnConstants` to keep the fences. I plan to address it separately. 
>> 
>> [1] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ref/Reference.java#L667
>> "HotSpot JVM retains the ref and does not GC it before a call to this method, because the JIT-compilers do not have GC-only safepoints."
>> 
>> Testing:
>> - [x] hs-tier1 - hs-tier8
>> - [x] hs-tier1 - hs-tier6 w/ -XX:+StressReachabilityFences -XX:+VerifyLoopOptimizations
>> - [x] java/lang/foreign microbenchmarks
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   IR test cases

@iwanowww I think this looks good to me now. Thanks for working on this!

Of course it is not perfect yet. There are a lot of follow⁻up RFE's filed. But it is already a larger patch, and it seems to be a step in the right direction.

One that I think we should not let sit very long - and possibly could even fix before integrating the change here:
[JDK-8370133](https://bugs.openjdk.org/browse/JDK-8370133) C2: Manage non-debug safepoint edges in structural manner

There are now a few places where you have special logic to deal with other SafePoint edges, and it seems very hacky and ad-hoc. It is more code, more implicit assumptions, and probably also prone for bugs. If we are going to integrate this fix here before addressing that messy SafePoint edge code, we should clean it up really soon, preferrably in the same release cycle (JDK27).

A **second reviewer** should also do a thorough review. I can't say I fully comprehend all implications here. There are some implicit assumptions that make me a bit nervous, but we probably just have to live with that - but we should make sure we are explicit about what assumptions we make and document them well.

src/hotspot/share/opto/phaseX.cpp line 2051:

> 2049:     //   java -XX:VerifyIterativeGVN=1000 -Xcomp -XX:+StressReachabilityFences
> 2050:     return false;
> 2051:   }

Is this still true?

src/hotspot/share/opto/reachability.cpp line 73:

> 71:  *
> 72:  * After loop opts are over, it becomes possible to reliably enumerate all interfering safe points and
> 73:  * to ensure that the referent is present in their oop maps.

For the sake of someone else who has to fix a bug here, or considers changing the design, can you please:
- Give a more concrete example, e.g: load of a native memory address is hoisted, ... explain that this means we move it over the SafePoint in the backedge, and why this is problematic.
- State your assumption / invariants that should hold after loop opts, that guarantee that it is safe to now attach to SP instead of RF. This one makes me a bit nervous, because it is another implicit assumption in C2, but I suppose we just have to live with that. But at least we can document it well ;)

src/hotspot/share/opto/reachability.cpp line 166:

> 164:     lpt->_reachability_fences = new Node_List();
> 165:   }
> 166:   lpt->_reachability_fences->push(new_rf);

This code is duplicated elsewhere. Consider refactoring it with a `lpt->reachability_fences_push` method that automatically allocates the new `Node_List`.

src/hotspot/share/opto/reachability.cpp line 216:

> 214:   // ResourceMark rm; // NB! not safe because insert_rf may trigger _idom reallocation
> 215:   Unique_Node_List redundant_rfs;
> 216:   GrowableArray<Pair<Node*,Node*>> worklist;

Tech debt alarm ;)

We should probably more `_idom` to a different arena then, right?

src/hotspot/share/opto/reachability.cpp line 223:

> 221:     IdealLoopTree* lpt = get_loop(rf);
> 222:     Node* referent = rf->referent();
> 223:     Node* loop_exit = lpt->unique_loop_exit_or_null();

Suggestion:

    IfFalseNode* loop_exit = lpt->unique_loop_exit_or_null();

src/hotspot/share/opto/reachability.cpp line 473:

> 471:       if (extra_edge != nullptr) {
> 472:         sfpt->add_req(extra_edge); // Add valid_length_test_input edge back
> 473:       }

Could it be that you have two meanings for "extra edge" here? OR does the top comment:

> Turn extra safepoint edges into reachability fences

match with this?

> sfpt->add_req(extra_edge); // Add valid_length_test_input edge back

Again: mixing up these edges really feels like tech debt. We should fix that soon.

test/hotspot/jtreg/compiler/c2/TestReachabilityFenceFlags.java line 48:

> 46:  *                   -XX:+StressReachabilityFences -XX:+OptimizeReachabilityFences -XX:+PreserveReachabilityFencesOnConstants
> 47:  *                     compiler.c2.TestReachabilityFenceFlags
> 48:  */

Conside dropping `-Xcomp -XX:-TieredCompilation`, because we will run with that at some point in our CI anyway. Would give more options for different kinds of compilation, right?

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25315#pullrequestreview-3551106049
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2597856271
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2597906466
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2597925796
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2597936186
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2597939958
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2597992100
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2598006581