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

[Emanuel Peter]

On Fri, 7 Nov 2025 10:30:46 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   update
>
> 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?

Hmm, but this here is only a component of the `enumerate_interfering_sfpts` below, which has essencially that name.

So maybe it should be `collect_interfering_sfpt_candidates_for_node` here and just `collect_interfering_sfpt_candidates` below?

> src/hotspot/share/opto/reachability.cpp line 409:
> 
>> 407:   visited.set(referent_ctrl->_idx); // end point
>> 408: 
>> 409:   Node_Stack stack(0);
> 
> `ResouceMark`?

We call this many times, so not sure if this could explode somehow?

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

And why not use a `Unique_Node_List`, to unite the `visited` and `stack` into a single `worklist`?

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

And what if we find a lot of SafePoints for each RF? Do we end up attaching quadratically many referent edges over all?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502686333
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502689245
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502736689
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2502848017