RFR: 8344049: Shenandoah: Eliminate init-update-refs safepoint

[Y. Srinivas Ramakrishna]

On Wed, 11 Dec 2024 19:08:08 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.

Looks good... Left a few documentation request comments. I haven't fully wrapped my head around the correctness of this yet (sorry, slow start to the new year :-), and will go over it again and complete it a bit later today after I get to the office.

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

> 1164:   }
> 1165: 
> 1166:   if (VerifyAfterGC) {

What are the conventions of when to use Verify{Before,After,During}GC on the one hand, vs ShenandoahVerify, G1Verify* etc., on the other?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2650:

> 2648: bool ShenandoahHeap::is_gc_state(GCState state) const {
> 2649:   return _gc_state_changed ? _gc_state.is_set(state) : ShenandoahThreadLocalData::is_gc_state(state);
> 2650: }

This needs a documentation comment, please; e.g. why we check `_gc_state_changed` before we check the global state. Is the transition of the local and global states wrt the phase described in a comment somewhere else already?

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 371:

> 369: public:
> 370:   char gc_state() const;
> 371:   bool is_gc_state(GCState state) const;

Can you write a 1-line documentation comment for this method? It would make its implementation clearer. (See my comment in the method's implementation.)

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 374:

> 372: 
> 373:   // This copies the global gc state into a thread local variable for all threads.
> 374:   // It is primarily intended to support quick access at barriers. All threads are

Instead of "It ..." say "The thread local gc state ..."

src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounters.cpp line 150:

> 148:     return 3;
> 149:   }
> 150:   if (heap->is_concurrent_mark_in_progress() || heap->is_concurrent_weak_root_in_progress() || heap->is_full_gc_in_progress()) {

naive question: where are the counters/encoding used?

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

PR Review: https://git.openjdk.org/jdk/pull/22688#pullrequestreview-2536114812
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1907457433
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1906544298
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1906551170
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1906548845
PR Review Comment: https://git.openjdk.org/jdk/pull/22688#discussion_r1906541100