From f402628e14322eababd650d2a2a9116922508a72 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Mon, 3 Feb 2025 12:23:22 -0800 Subject: [PATCH 1/3] Set gc state for all attached threads (not just java threads). --- src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp index 10dc344657fe4..37470067ed9b8 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp @@ -113,8 +113,10 @@ void ShenandoahBarrierSet::on_thread_attach(Thread *thread) { assert(queue.buffer() == nullptr, "SATB queue should not have a buffer"); assert(queue.index() == 0, "SATB queue index should be zero"); queue.set_active(_satb_mark_queue_set.is_active()); + + ShenandoahThreadLocalData::set_gc_state(thread, _heap->gc_state()); + if (thread->is_Java_thread()) { - ShenandoahThreadLocalData::set_gc_state(thread, _heap->gc_state()); ShenandoahThreadLocalData::initialize_gclab(thread); BarrierSetNMethod* bs_nm = barrier_set_nmethod(); From 1a4e3bb1b2066d3284b0cadf0865075a1fb93a09 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Mon, 10 Feb 2025 13:51:26 -0800 Subject: [PATCH 2/3] Hold the thread lock when concurrently changing gc state --- .../share/gc/shenandoah/shenandoahHeap.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index db08014792c64..ab4c0f2e2ca12 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -1262,11 +1262,19 @@ void ShenandoahHeap::evacuate_collection_set(bool concurrent) { } void ShenandoahHeap::concurrent_prepare_for_update_refs() { - // It's possible that evacuation succeeded, but we could still be cancelled when we get here. - // A cancellation at this point means the degenerated cycle must resume from update-refs. - set_gc_state_concurrent(EVACUATION, false); - set_gc_state_concurrent(WEAK_ROOTS, false); - set_gc_state_concurrent(UPDATE_REFS, true); + { + // Taking the thread lock here assures that any thread created after we change the gc + // state will have the correct state. It also prevents attaching threads from seeing + // an inconsistent state. If another thread holds this lock while it is being created, + // they will have the wrong GC state, but they will be added to the list of java threads + // and so will be corrected by the handshake. + MutexLocker lock(Threads_lock); + + // A cancellation at this point means the degenerated cycle must resume from update-refs. + set_gc_state_concurrent(EVACUATION, false); + set_gc_state_concurrent(WEAK_ROOTS, false); + set_gc_state_concurrent(UPDATE_REFS, true); + } // This will propagate the gc state and retire gclabs and plabs for threads that require it. ShenandoahPrepareForUpdateRefs prepare_for_update_refs(_gc_state.raw_value()); From c57bf8a03c0706cf2a7140030679261d67d13226 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Tue, 11 Feb 2025 11:36:06 -0800 Subject: [PATCH 3/3] Update comments, add an assertion --- .../share/gc/shenandoah/shenandoahHeap.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index ab4c0f2e2ca12..68b83cbf75a87 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -1263,11 +1263,10 @@ void ShenandoahHeap::evacuate_collection_set(bool concurrent) { void ShenandoahHeap::concurrent_prepare_for_update_refs() { { - // Taking the thread lock here assures that any thread created after we change the gc - // state will have the correct state. It also prevents attaching threads from seeing - // an inconsistent state. If another thread holds this lock while it is being created, - // they will have the wrong GC state, but they will be added to the list of java threads - // and so will be corrected by the handshake. + // Java threads take this lock while they are being attached and added to the list of thread. + // If another thread holds this lock before we update the gc state, it will receive a stale + // gc state, but they will have been added to the list of java threads and so will be corrected + // by the following handshake. MutexLocker lock(Threads_lock); // A cancellation at this point means the degenerated cycle must resume from update-refs. @@ -2033,6 +2032,12 @@ void ShenandoahHeap::set_gc_state_at_safepoint(uint mask, bool value) { } void ShenandoahHeap::set_gc_state_concurrent(uint mask, bool value) { + // Holding the thread lock here assures that any thread created after we change the gc + // state will have the correct state. It also prevents attaching threads from seeing + // an inconsistent state. See ShenandoahBarrierSet::on_thread_attach for reference. Established + // threads will use their thread local copy of the gc state (changed by a handshake, or on a + // safepoint). + assert(Threads_lock->is_locked(), "Must hold thread lock for concurrent gc state change"); _gc_state.set_cond(mask, value); }