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

Mon Sep 8 13:34:26 UTC 2025

On Wed, 3 Sep 2025 21:29:43 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:
> 
>   whitespaces

Thanks for all the updates. I went over all your responses quickly, but still need to read through the description in `reachability.cpp` now.

I'll do another pass over everything once you address my responses ;)

src/hotspot/share/opto/callGenerator.cpp line 623:

> 621:       return; // keep the original call node as the holder of reachability info
> 622:     }
> 623:   }

Maybe that's just me. But people use the assert messages both in positive and negative ways, and so this is a bit ambiguous. Maybe you can write:
`no reachability edge should be present`

I'm still a bit unsure what the `SafePointNode::grow_stack` comment means.
In the previous comment https://github.com/openjdk/jdk/pull/25315#discussion_r2320120466 you explained more. Why not add that here instead?

src/hotspot/share/opto/compile.cpp line 2522:

> 2520:     if (failing())  return;
> 2521:     assert(_reachability_fences.length() == 0, "no RF nodes allowed");
> 2522:   }

Looks better than before :)

I'm still wondering: do we need to do a whole loop-opts phase here? It probably has a performance impact, right?
Have you measured that?

If it is measurable: could we just go through `_reachability_fences`, and hack the graph and clean up with IGVN? Or do we really need the loop state to do this successfully?

src/hotspot/share/opto/loopTransform.cpp line 66:

> 64: //------------------------------unique_loop_exit_or_null----------------------
> 65: // Return the loop-exit projection if it is unique.
> 66: Node* IdealLoopTree::unique_loop_exit_or_null() {

I suggested it here:
https://github.com/openjdk/jdk/pull/25315#discussion_r2149677594
Can we change the return type to `IfProjNode`?

Also: when is it possible that there are none or multiple loop exits?
Can you add a comment below where you return nullptr?

src/hotspot/share/opto/macro.cpp line 973:

> 971:         _igvn._worklist.push(ac);
> 972:       } else if (use->is_ReachabilityFence() && OptimizeReachabilityFences) {
> 973:         use->as_ReachabilityFence()->clear_referent(_igvn); // redundant fence

Thanks for refactoring a bit here :)

Is this rf guaranteed to belong to the Allocation somehow?

src/hotspot/share/opto/parse1.cpp line 2233:

> 2231:       insert_reachability_fence(referent);
> 2232:     }
> 2233:   }

Comments look better, thanks :)

But `StressReachabilityFences` seems to promise that it should happen randomly. Did you want to do that or adjust the flag comment?

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

> 134:     return true;
> 135:   }
> 136: }

Nit: `an no-op` -> `a no-op`

Also: do you need the return value? The only use case does not do anything with it.

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

> 436:   if (!OptimizeReachabilityFences) {
> 437:     return false;
> 438:   }

Can this ever fail? Could it be an assert?

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

> 439: 
> 440:   Unique_Node_List redundant_rfs;
> 441:   Node_List worklist;

Not sure if necessary, but maybe good practice anyway: add `ResourceMark`.

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

> 451:         SafePointNode* sfpt = safepoints.pop()->as_SafePoint();
> 452:         assert(is_dominator(get_ctrl(referent), sfpt), "");
> 453:         assert(sfpt->req() == rf_start_offset(sfpt), "");

Is this the only reason we need this to happend during LoopOpts - i.e. that we can call `get_ctrl` and `is_dominator`?

Because it is potentially a lot of overhead to create the whole loop-opts structures just for this.

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

PR Review: https://git.openjdk.org/jdk/pull/25315#pullrequestreview-3196301873
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330095168
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330176841
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330209593
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330230044
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330256973
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330221500
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330181204
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330188708
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2330192891