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

Mon Jun 16 09:51:39 UTC 2025

On Fri, 23 May 2025 22:43:35 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:
> 
>   renaming

I'm on my first pass through the code, just leaving a few first observations / comments :)

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

> 615:   uint endoff = call->jvms()->endoff();
> 616:   if (C->inlining_incrementally()) {
> 617:     assert(endoff == call->req(), ""); // assert in SafePointNode::grow_stack

What exactly are you asserting here? And what is the comment for?

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

> 618:   } else {
> 619:     if (call->req() > endoff) {
> 620:       assert(OptimizeReachabilityFences, "");

Can you please add a helpful assert message?

src/hotspot/share/opto/callnode.cpp line 950:

> 948:               case CatchProjNode::catch_all_index:    projs->catchall_catchproj    = cpn; break;
> 949:               default: {
> 950:                 assert(cpn->_con > 1, ""); // exception table; rethrow case

Can we please turn this into a helpful assert message?

src/hotspot/share/opto/callnode.hpp line 497:

> 495:   // Are we guaranteed that this node is a safepoint?  Not true for leaf calls and
> 496:   // for some macro nodes whose expansion does not have a safepoint on the fast path.
> 497:   virtual bool guaranteed_safepoint()  { return true; }

I see you only copied it. It makes me a little nervous when we call the "default" case safe. Because when you add more cases, you just assume it is safe... and if it is not we first have to discover that through a bug. What do you think?

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

> 2524:     print_method(PHASE_ELIMINATE_REACHABILITY_FENCES, 2);
> 2525:     if (failing())  return;
> 2526:   }

You will have to check the impact on compile time here. Running an extra round of loop opts might be significant.

Do you really need to use a loop opts phase for this? Or would something like `process_for_post_loop_opts_igvn` work for it?

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

> 3956:     Node* rf = C->reachability_fence(i);
> 3957:     Node* in = rf->in(1);
> 3958:     if (in->is_DecodeN()) {

Why not:
Suggestion:

    ReachabilityFence* rf = C->reachability_fence(i);
    DecodeNNode* dn = rf->in(1)->isa_DecodeN();
    if (dn != nullptr) {

src/hotspot/share/opto/compile.hpp line 381:

> 379:   GrowableArray<OpaqueTemplateAssertionPredicateNode*>  _template_assertion_predicate_opaques;
> 380:   GrowableArray<Node*>  _expensive_nodes;       // List of nodes that are expensive to compute and that we'd better not let the GVN freely common
> 381:   GrowableArray<Node*>  _reachability_fences;   // List of reachability fences

Why not:
Suggestion:

  GrowableArray<ReachabilityFenceNode*>  _reachability_fences;   // List of all reachability fences

src/hotspot/share/opto/compile.hpp line 741:

> 739:   void remove_reachability_fence(Node* n) {
> 740:     _reachability_fences.remove_if_existing(n);
> 741:   }

You could also add the type `ReachabilityFenceNode*` here.

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

PR Review: https://git.openjdk.org/jdk/pull/25315#pullrequestreview-2931260265
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149442595
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149444306
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149464647
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149488305
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149503924
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149514942
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149518776
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2149519545