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

[Y. Srinivas Ramakrishna]

On Fri, 10 Jan 2025 19:49:17 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Shenandoah typically takes 4 safepoints per GC cycle. Although Shenandoah itself does not spend much time on these safepoints, it may still take quite some time for all of the mutator threads to reach the safepoint. The occasionally long time-to-safepoint increases latency in the higher percentiles.
>> 
>> The `init-update-refs` safepoint is responsible for retiring GCLABs (and PLABs) used during evacuation. Once evacuation is complete, no threads will access these LABs. This need not be done on a safepoint. `init-update-refs` is also where the global and thread local copies of the `gc_state` are updated. However, here we are turning off the `WEAK_ROOTS` flag _after_ all of the unmarked weak referents have been `nulled` out, so this does not need to happen atomically with respect to the mutators. Neither is it necessary to change the other state flags (EVACUATION, UPDATE_REFS) atomically across all mutators.
>> 
>> Note that the `init-update-refs` safepoint is still taken if either verification or `ShenandoahPacing` are enabled.
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve comments and method names

Left a few more comments, and will make one more final readthrough and approve. Thanks for your continued patience with my slow review!

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 196:

> 194: 
> 195:     // Evacuation is complete, retire gc labs
> 196:     heap->concurrent_prepare_for_update_refs();

For consistency with other related method naming, can we use "updaterefs" instead of "update_refs" (makes IDE searches easier to locate related methods).

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.

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.

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

PR Review: https://git.openjdk.org/jdk/pull/22688#pullrequestreview-2544388098
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1911793884
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1911797849
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1911803283