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

[William Kemper]

On Sat, 11 Jan 2025 01:50:39 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Improve comments and method names
>
> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1245:
> 
>> 1243:   void do_thread(Thread* thread) override {
>> 1244:     _propagator.do_thread(thread);
>> 1245:     if (ShenandoahThreadLocalData::gclab(thread) != nullptr) {
> 
> Which thread may have this be null? (I am looking at the ShenandoahRetireGCLabClosure which insists that this should be non-null.)
> 
> I assume we have some threads here that have a gc state that must be updated but which don't have a gc lab.
> 
> I am wondering if the check for an initialized gclab and in the generational case the plab can be pushed down into the closure rather than being exposed here. At that place, we would want to document (or as needed assert) why some threads targeted by the closure may have null gclab or plab.

Only worker threads and java threads are required to have gclabs. In other use cases (`shHeap::make_labs_parsable`, `shHeap::retire_gclabs`), this closure is _only_ used on java and worker threads, so pushing the test into the closure would be redundant for other uses. I will put in a comment here instead?

Additionally, I noticed an inconsistency between `make_labs_parsable` (which skips the safepoint workers) and `retire_gclabs` (which also visited the control thread). An earlier version of this PR had given gclabs to the control and vm threads, but these threads will only perform evacuations in very rare circumstances, so I've removed their gclabs.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1267:
> 
>> 1265: 
>> 1266:   // This will propagate the gc state and retire gclabs and plabs for threads that require it.
>> 1267:   ShenandoahPrepareForUpdateRefs prepare_for_update_refs(_gc_state.raw_value());
> 
> In looking at this I see that we do not set `_gc_state_changed` here because we don't want individual threads to observe the global state, but only their local state (when it's propagated below). It would be good to emphasise this in the documetation of `_gc_state_changed` use protocol.
> 
> Indeed, as I had suggested before, I think this might be better encapsulated with a `set_gc_state_concurrent()` that is analogous to `set_gc_state_at_safepoint()` that takes the appropriate state value as an argument, and uses the appropriate `_gc_state_changed` protocol.
> 
> IIUC, this will be re-used when other safepoints are eliminated in the future.

I'll encapsulate the access here and improve the documentation for `_gc_state_changed`.

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

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