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

Wed Sep 3 20:43:52 UTC 2025

On Tue, 3 Jun 2025 17:20:38 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   renaming
>
> src/hotspot/share/opto/c2_globals.hpp line 83:
> 
>> 81:                                                                             \
>> 82:   product(bool, StressReachabilityFences, false, DIAGNOSTIC,                \
>> 83:           "Randomly insert ReachabilityFence nodes")                        \
> 
> Drive-by sniping: what about a hello-world test where you test out these flags?

Good idea. Added one.

> 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?

The assert ensures there are no reachability edges present when incremental inlining takes place. Inlining logic doesn't expect any extra edges past debug info and the comment refers to the assert which fires the first.

> 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?

Well, it's a SafePointNode class after all. I lifted it from `CallNode` subclass to avoid elaborate check on SafePoint nodes (!is_Call() || as_Call() && guaranteed_safepoint()`)).

If some node extends SafePointNode, but doesn't keep JVM state, it has to communicate it to users one way or another. And changing the default doesn't improve the situation IMO: reporting a safepoint node as a non-safepoint is still a bug.

> 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) {

Ok, reshaped as you suggested.

> 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

Ok, done.

> 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.

Done.

> src/hotspot/share/opto/loopTransform.cpp line 78:
> 
>> 76:   }
>> 77:   return unique_loop_exit;
>> 78: }
> 
> `proj_out_or_null` returns a `ProjNode` (it is probably a `IfTrue` or `IfFalse`, right?) and `outer_loop_exit` returns a `IfFalseNode`. So we should be able to return a `IfProjNode` from this method. What do you think?
> 
> What is the benefit of the `unique_loop_exit` variable here? Why not return immediately?

It was easier to inspect it in the debugger. Reshaped as you suggested.

> src/hotspot/share/opto/macro.cpp line 983:
> 
>> 981:         _igvn._worklist.push(ac);
>> 982:       } else if (use->is_ReachabilityFence() && OptimizeReachabilityFences) {
>> 983:         _igvn.replace_input_of(use, 1, _igvn.makecon(TypePtr::NULL_PTR)); // reset; redundant fence
> 
> Can you quickly explain in a code comment how this does a "reset"? What happens with it next?

Turned it into `ReachabilityFenceNode::clear_referent()`. Hope it makes it clearer.

> src/hotspot/share/opto/node.hpp line 701:
> 
>> 699:       DEFINE_CLASS_ID(MemBar,      Multi, 3)
>> 700:         DEFINE_CLASS_ID(Initialize,        MemBar, 0)
>> 701:         DEFINE_CLASS_ID(MemBarStoreStore,  MemBar, 1)
> 
> Suggestion:
> 
>         DEFINE_CLASS_ID(Initialize,       MemBar, 0)
>         DEFINE_CLASS_ID(MemBarStoreStore, MemBar, 1)
> 
> I don't think you needed to touch the lines above, right?

Fixed.

> src/hotspot/share/opto/parse.hpp line 361:
> 
>> 359:   bool          _wrote_fields;       // Did we write any field?
>> 360:   Node*         _alloc_with_final_or_stable; // An allocation node with final or @Stable field
>> 361:   Node*         _stress_rf_hook; // StressReachabilityFences support
> 
> You could write out the `rf`

I'd like to avoid that. `_stress_reachability_fence_hook` is way too verbose IMO. The declaration and all the accesses are accompanied by `StressReachabilityFences` which should make it clear what `rf` refers to.

> src/hotspot/share/opto/parse1.cpp line 379:
> 
>> 377:         _stress_rf_hook->add_req(loc);
>> 378:       }
>> 379:     }
> 
> Can you add a short code comment describing what you are doing here, please?

Done.

> src/hotspot/share/opto/parse1.cpp line 394:
> 
>> 392:         _stress_rf_hook->add_req(stk);
>> 393:       }
>> 394:     }
> 
> A short code comment would be helpful

Done.

> src/hotspot/share/opto/parse1.cpp line 2222:
> 
>> 2220: 
>> 2221:   if (StressReachabilityFences) {
>> 2222:     // Keep all oop arguments alive until method return.
> 
> Why? Can you extend the comment a little?

Done. Does it look better now?

> src/hotspot/share/opto/reachability.cpp line 44:
> 
>> 42:  *   (0) initial set of RFs is materialized during parsing;
>> 43:  *   (1) optimization pass during loop opts which eliminates redundant nodes and
>> 44:  *     moves loop-invariant ones outside loops;
> 
> Suggestion:
> 
>  *   (1) optimization pass during loop opts which eliminates redundant nodes and
>  *       moves loop-invariant ones outside loops;
> 
> I'd prever consistent indentation, but optional/question of taste

Fixed.

> src/hotspot/share/opto/reachability.cpp line 51:
> 
>> 49:  *
>> 50:  * It looks attractive to get rid of RF nodes early and transfer to safepoint-attached representation,
>> 51:  * but it is not correct until loop opts are done.
> 
> Why is it not correct? What could go wrong? Why is it safe to do it after loop opts?

Live ranges of values are routinely extended during loop opts. And it can break the invariant that all interfering safepoints contain the referent in their oop map. (If an interfering safepoint doesn't keep the referent alive, then it becomes possible for the referent to be prematurely GCed.)  

After loop opts are over, it becomes possible to reliably enumerate all interfering safe points and ensure the referent present in their oop maps.

> test/hotspot/jtreg/compiler/c2/TestReachabilityFence.java line 38:
> 
>> 36:  * @summary Tests to ensure that reachabilityFence() correctly keeps objects from being collected prematurely.
>> 37:  * @modules java.base/jdk.internal.misc
>> 38:  * @run main/othervm -Xbatch compiler.c2.TestReachabilityFence
> 
> What about some extra runs where you use your new flags?

This particular test is carefully crafted to provoke a failure when reachability fence effects aren't properly modeled. Stressing RF implementation doesn't help here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320090697
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320120466
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320062127
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320121852
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320122602
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320123063
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320123818
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320135080
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320135683
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320066556
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320136667
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320137310
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320138235
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320138496
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320080872
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2320087314