RFR: 8344049: Shenandoah: Eliminate init-update-refs safepoint [v2]

[William Kemper]

On Fri, 10 Jan 2025 00:40:26 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 32 additional commits since the last revision:
>> 
>>  - Improve comments
>>  - Merge remote-tracking branch 'jdk/master' into remove-init-update-refs-safepoint
>>  - Fix comments
>>  - Fix comment, revert unnecessary change
>>  - Merge remote-tracking branch 'jdk/master' into remove-init-update-refs-safepoint
>>  - Fix phase encoding to handle weak roots
>>  - WIP: Use Threads::threads_do for propagating gc state (consolidated)
>>  - WIP: Use Threads::threads_do for propagating gc state
>>  - Remove unnecessary gc state propagations
>>  - Encapsulate gc state
>>  - ... and 22 more: https://git.openjdk.org/jdk/compare/34b4faa5...83ac7b49
>
> src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 368:
> 
>> 366:   // This updates the singular, global gc state. This call must happen on a safepoint.
>> 367:   // However, in some cases (init update refs, e.g.), the gc state may change concurrently
>> 368:   // and will be propagated to all threads by a handshake operation.
> 
> I am a little bit confused by the statement starting at "However, ...". Did you mean that the "local copy of the global state" may be changed outside of a safepoint but not the global state itself? 
> I notice that `set_gc_state()` still asserts that we are at a safepoint:
> https://github.com/openjdk/jdk/blob/83ac7b49d34081beb3ff58f1c159d22faacd077a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp#L2000
> 
> Ah, now I see that you use a different API for setting the global gc state outside of a safepoint.
> 
> If my understanding is correct, then we should probably rename the APIs such that the one that is expected to be set at a safepoint uses `set_gc_state_at_safepoint()` and the one that doesn't might use `set_gc_state_concurrent()` or something like that. That would be less confusing.
> 
> It also brings up the issue of what specific state predicates it's safe to test when. E.g. whether `is_gc_state()` can be safely tested any time during a safepoint or concurrently. I think it is safe, but explicitly stating this might be useful, not least because we seem to have one state change API that still asserts that we should be at a safepoint.

It is always safe to call `is_gc_state` because it uses the most recently changed value. That is, if the `_gc_state` changes on a safepoint, `is_gc_state` will use the value set during the safepoint. Otherwise, it uses the value which was either changed concurrently through the thread local handshake (init-update-refs) or the value which was propagated to all threads at the end of the safepoint.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1911094315