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

Fri Nov 7 11:13:27 UTC 2025

On Fri, 7 Nov 2025 07:14:29 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:
> 
>   update

First batch of today. I just responded to some of your responses, and dug into random places ;)

src/hotspot/share/opto/loopnode.cpp line 5147:

> 5145:     assert(do_expensive_nodes || do_optimize_reachability_fences, "why are we here?");
> 5146:     // Use a change to optimize reachability fence nodes irrespective of
> 5147:     // whether loop optimizations are performed or not.

What do you mean by `Use a change`?

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

> 86: // RF is redundant for some referent oop when the referent has another user which keeps it alive across the RF.
> 87: // In terms of dominance relation it can be formulated as "a referent has a user which is dominated by the redundant RF".
> 88: // Until loop opts are over, only RF nodes are considered as usages (controlled by rf_only flag).

Can you say why until after loop opts are over only RF are considered?

How does this play with allocation elimination etc? What if we run this after loop opts where we still have the allocation, but before the elimination. And then we eventually lose all referents. Could something like that happen?

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

> 106:       return true; // ignore fences on boxed primitives produced by valueOf methods
> 107:     }
> 108:   }

Nice, thanks for adding the comment. I'm trying to understand the reason why that is ok.

So someone would have set a RF for a boxed primitive. But we don't expect anything to ever be attached to a boxed primitive, and so we can just ignore these RF? Is that the reason? Might be worth writing it in a code comment explicitly.

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

> 184:     return false; // uncommon traps are exit points
> 185:   }
> 186:   return true;

Suggestion:

  // By default, we return a conservative answer, and assume it could interfere.
  return true;

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

> 381: // Linearly traverse CFG upwards starting at n until first merge point.
> 382: // All encountered safepoints are recorded in safepoints list.
> 383: static void linear_traversal(Node* n, Node_Stack& worklist, VectorSet& visited, Node_List& safepoints) {

The second comment line does not sound accurate, we don't collect ALL, only the candidates. Maybe we can find a better method name, and even remove that comment because of it?

Given the more useful sub query `is_interfering_sfpt_candidate`, I think we could name this method something like `collect_interfering_sfpt_candidates`. Or is it very important that this is a linear traversal vs some other traversal we could choose from?

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

> 382: // All encountered safepoints are recorded in safepoints list.
> 383: static void linear_traversal(Node* n, Node_Stack& worklist, VectorSet& visited, Node_List& safepoints) {
> 384:   for (Node* ctrl = n; ctrl != nullptr; ctrl = ctrl->in(0)) {

This "fast-forwarding" looks a bit like an optimization. Why not just add all CFG nodes on the worklist, would that not simplify the graph a little? Or did you find a case where this was really important?

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

> 407:   visited.set(referent_ctrl->_idx); // end point
> 408: 
> 409:   Node_Stack stack(0);

`ResouceMark`?

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

> 407:   visited.set(referent_ctrl->_idx); // end point
> 408: 
> 409:   Node_Stack stack(0);

Also: you now call it `stack` out here but `worklist` inside `linear_traversal`. I would use a consistent name.

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

> 439: 
> 440:   assert(OptimizeReachabilityFences, "required");
> 441:   assert(C->post_loop_opts_phase(), "required");

You could give the reason in the assert message ;)

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

> 444:   ResourceMark rm;
> 445:   Unique_Node_List redundant_rfs;
> 446:   Node_List worklist;

Looks like this `worklist` is really a list of `<sfpt,referent>` pairs.

Consider making it a `GrowableArray` with a pair type and also consider giving it a more descriptive name, calling it by whatever we collect in it. Maybe `sfpt_referent_pairs`?

Also: why do we need this worklist, and not just attach the referent to the sfpt eagerly? And: can it be that we add the same pair multiple times? Is that intentional?

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

> 459:     if (!is_redundant_rf(rf, false /*rf_only*/)) {
> 460:       Node_List safepoints;
> 461:       enumerate_interfering_sfpts(rf, this, safepoints);

Could this explode if we have a lot of RF in the graph? Do we need a ResouceMark, or reuse the `safeponts` node list?

Imagine something like this:

referent
if (flag1) { something } else { something }
...
if (flag100) { something } else { something }
if (x1) { RF(referent); }
...
if (x100) { RF(referent); }

So we would call `enumerate_interfering_sfpts` 100x, and then traverse the graph with about 100-400 nodes each time. You can see how that grows quadratically. Maybe that's fine for runtime, but is it also ok for memory?

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

PR Review: https://git.openjdk.org/jdk/pull/25315#pullrequestreview-3432711599
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502498455
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502534061
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502617200
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502647790
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502669143
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502796906
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502680412
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502724464
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502837337
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502778969
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502745140