From 0497cca00dd1b87e8bce064d33f72e6ad7a2ce44 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Thu, 9 Oct 2025 13:17:55 -0700 Subject: [PATCH 01/11] Split SuspendibleThreadSet::synchronize into two steps: 1. SuspendibleThreadSet::synchronize_begin set _suspend_all falg; 2.SuspendibleThreadSet::synchronize wait for all threads to synchronize --- src/hotspot/share/gc/shared/suspendibleThreadSet.cpp | 10 +++++++++- src/hotspot/share/gc/shared/suspendibleThreadSet.hpp | 5 ++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index 83783b31ad996..fd5a816e578b9 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -89,14 +89,22 @@ void SuspendibleThreadSet::yield_slow() { } } -void SuspendibleThreadSet::synchronize() { +void SuspendibleThreadSet::synchronize_begin() { if (ConcGCYieldTimeout > 0) { _suspend_all_start = os::elapsedTime(); } + { MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); assert(!should_yield(), "Only one at a time"); AtomicAccess::store(&_suspend_all, true); + } +} + +void SuspendibleThreadSet::synchronize() { + assert(AtomicAccess::load(&_suspend_all), "synchronize must have begun."); + { + MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); if (is_synchronized()) { return; } diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp index 38568c015bc2f..2e8e56b0c25de 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp @@ -68,7 +68,10 @@ class SuspendibleThreadSet : public AllStatic { } } - // Returns when all threads in the set are suspended. + // begin to synchronize suspendible threads, + static void synchronize_begin(); + + // synchronize all suspendible threads static void synchronize(); // Resumes all suspended threads in the set. From b1e96fc14e72bdd68dd2df90700e27ca63e76f5b Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Thu, 9 Oct 2025 13:38:44 -0700 Subject: [PATCH 02/11] Add CollectedHeap::safepoint_synchronize --- src/hotspot/share/gc/g1/g1CollectedHeap.cpp | 6 +++++- src/hotspot/share/gc/g1/g1CollectedHeap.hpp | 3 +++ src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp | 6 ++++++ src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp | 3 +++ src/hotspot/share/gc/serial/serialHeap.cpp | 6 ++++++ src/hotspot/share/gc/serial/serialHeap.hpp | 3 +++ src/hotspot/share/gc/shared/collectedHeap.hpp | 1 + src/hotspot/share/gc/shared/suspendibleThreadSet.cpp | 2 ++ src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp | 4 ++++ src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp | 3 +++ src/hotspot/share/gc/z/zCollectedHeap.cpp | 4 ++++ src/hotspot/share/gc/z/zCollectedHeap.hpp | 3 +++ src/hotspot/share/runtime/safepoint.cpp | 1 + 13 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp index 42122f42afb44..cc781ce43cebb 100644 --- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp +++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp @@ -1570,11 +1570,15 @@ void G1CollectedHeap::stop() { } void G1CollectedHeap::safepoint_synchronize_begin() { - SuspendibleThreadSet::synchronize(); + SuspendibleThreadSet::synchronize_begin(); _last_synchronized_start = os::elapsed_counter(); } +void G1CollectedHeap::safepoint_synchronize() { + SuspendibleThreadSet::synchronize(); +} + void G1CollectedHeap::safepoint_synchronize_end() { jlong now = os::elapsed_counter(); jlong synchronize_duration = now - _last_synchronized_start; diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.hpp b/src/hotspot/share/gc/g1/g1CollectedHeap.hpp index 845f8a257a1da..ad17552e09450 100644 --- a/src/hotspot/share/gc/g1/g1CollectedHeap.hpp +++ b/src/hotspot/share/gc/g1/g1CollectedHeap.hpp @@ -910,6 +910,9 @@ class G1CollectedHeap : public CollectedHeap { jint initialize() override; void safepoint_synchronize_begin() override; + + void safepoint_synchronize() override; + void safepoint_synchronize_end() override; jlong last_refinement_epoch_start() const { return _last_refinement_epoch_start; } diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp index ae2c36e44c56f..2ebe66bceb227 100644 --- a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp +++ b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp @@ -153,6 +153,12 @@ void ParallelScavengeHeap::initialize_serviceability() { } void ParallelScavengeHeap::safepoint_synchronize_begin() { + if (UseStringDeduplication) { + SuspendibleThreadSet::synchronize_begin(); + } +} + +void ParallelScavengeHeap::safepoint_synchronize() { if (UseStringDeduplication) { SuspendibleThreadSet::synchronize(); } diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp index fea827430caca..4e8f4093dc0d5 100644 --- a/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp +++ b/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp @@ -160,6 +160,9 @@ class ParallelScavengeHeap : public CollectedHeap { jint initialize() override; void safepoint_synchronize_begin() override; + + void safepoint_synchronize() override; + void safepoint_synchronize_end() override; void post_initialize() override; diff --git a/src/hotspot/share/gc/serial/serialHeap.cpp b/src/hotspot/share/gc/serial/serialHeap.cpp index dbd54c302ea3e..2144f16097e09 100644 --- a/src/hotspot/share/gc/serial/serialHeap.cpp +++ b/src/hotspot/share/gc/serial/serialHeap.cpp @@ -146,6 +146,12 @@ GrowableArray SerialHeap::memory_pools() { } void SerialHeap::safepoint_synchronize_begin() { + if (UseStringDeduplication) { + SuspendibleThreadSet::synchronize_begin(); + } +} + +void SerialHeap::safepoint_synchronize() { if (UseStringDeduplication) { SuspendibleThreadSet::synchronize(); } diff --git a/src/hotspot/share/gc/serial/serialHeap.hpp b/src/hotspot/share/gc/serial/serialHeap.hpp index 388da13b1b0ed..01f57a7eeca6d 100644 --- a/src/hotspot/share/gc/serial/serialHeap.hpp +++ b/src/hotspot/share/gc/serial/serialHeap.hpp @@ -259,6 +259,9 @@ class SerialHeap : public CollectedHeap { OldGenScanClosure* old_cl); void safepoint_synchronize_begin() override; + + void safepoint_synchronize() override; + void safepoint_synchronize_end() override; // Support for loading objects from CDS archive into the heap diff --git a/src/hotspot/share/gc/shared/collectedHeap.hpp b/src/hotspot/share/gc/shared/collectedHeap.hpp index 0516091228309..db514714f8794 100644 --- a/src/hotspot/share/gc/shared/collectedHeap.hpp +++ b/src/hotspot/share/gc/shared/collectedHeap.hpp @@ -257,6 +257,7 @@ class CollectedHeap : public CHeapObj { // Stop and resume concurrent GC threads interfering with safepoint operations virtual void safepoint_synchronize_begin() {} + virtual void safepoint_synchronize() {} virtual void safepoint_synchronize_end() {} void add_vmthread_cpu_time(jlong time); diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index fd5a816e578b9..5a5ec8863bd78 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -122,7 +122,9 @@ void SuspendibleThreadSet::synchronize() { // being signaled until we get back here again for some later // synchronize call. Hence, there is no need to re-check for // is_synchronized after the wait; it will always be true there. + log_trace(safepoint)("Waiting for %d GC threads to block", _nthreads - _nthreads_stopped); _synchronize_wakeup->wait(); + log_trace(safepoint)("All GC threads have been blocked"); #ifdef ASSERT MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index b2fd32d2fd0b9..94c41b8644821 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -2699,6 +2699,10 @@ bool ShenandoahHeap::is_uncommit_in_progress() { void ShenandoahHeap::safepoint_synchronize_begin() { StackWatermarkSet::safepoint_synchronize_begin(); + SuspendibleThreadSet::synchronize_begin(); +} + +void ShenandoahHeap::safepoint_synchronize() { SuspendibleThreadSet::synchronize(); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp index 11a20d4a2f99f..5980c327a9d71 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp @@ -666,6 +666,9 @@ class ShenandoahHeap : public CollectedHeap { // public: void safepoint_synchronize_begin() override; + + void safepoint_synchronize() override; + void safepoint_synchronize_end() override; // ---------- Code roots handling hooks diff --git a/src/hotspot/share/gc/z/zCollectedHeap.cpp b/src/hotspot/share/gc/z/zCollectedHeap.cpp index ae60219139cea..0ebde9afe8561 100644 --- a/src/hotspot/share/gc/z/zCollectedHeap.cpp +++ b/src/hotspot/share/gc/z/zCollectedHeap.cpp @@ -335,6 +335,10 @@ void ZCollectedHeap::safepoint_synchronize_begin() { StackWatermarkSet::safepoint_synchronize_begin(); ZGeneration::young()->synchronize_relocation(); ZGeneration::old()->synchronize_relocation(); + SuspendibleThreadSet::synchronize_begin(); +} + +void ZCollectedHeap::safepoint_synchronize() { SuspendibleThreadSet::synchronize(); } diff --git a/src/hotspot/share/gc/z/zCollectedHeap.hpp b/src/hotspot/share/gc/z/zCollectedHeap.hpp index bbcddec917f21..e909757a9f22f 100644 --- a/src/hotspot/share/gc/z/zCollectedHeap.hpp +++ b/src/hotspot/share/gc/z/zCollectedHeap.hpp @@ -110,6 +110,9 @@ class ZCollectedHeap : public CollectedHeap { bool contains_null(const oop* p) const override; void safepoint_synchronize_begin() override; + + void safepoint_synchronize() override; + void safepoint_synchronize_end() override; void pin_object(JavaThread* thread, oop obj) override; diff --git a/src/hotspot/share/runtime/safepoint.cpp b/src/hotspot/share/runtime/safepoint.cpp index b44a43bbfb181..e93f12db228e9 100644 --- a/src/hotspot/share/runtime/safepoint.cpp +++ b/src/hotspot/share/runtime/safepoint.cpp @@ -373,6 +373,7 @@ void SafepointSynchronize::begin() { // Will spin until all threads are safe. int iterations = synchronize_threads(safepoint_limit_time, nof_threads, &initial_running); + Universe::heap()->safepoint_synchronize(); assert(_waiting_to_block == 0, "No thread should be running"); #ifndef PRODUCT From bd1d7f28383d3a7e3d64ccee6735a61b8e8d185e Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Thu, 9 Oct 2025 15:31:59 -0700 Subject: [PATCH 03/11] 1. always call SuspendibleThreadSet::synchronize_begin before SuspendibleThreadSet::synchronize 2. Make access of SuspendibleThreadSet::_nthreads and SuspendibleThreadSet::_nthreads_stopped atomic --- .../share/gc/g1/g1ConcurrentRefine.cpp | 1 + .../share/gc/shared/suspendibleThreadSet.cpp | 22 +++++++++---------- .../share/gc/shared/suspendibleThreadSet.hpp | 9 ++++++-- src/hotspot/share/gc/z/zGeneration.cpp | 1 + 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp b/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp index ed6a9ad42928a..aa8023e3b0d2e 100644 --- a/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp +++ b/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp @@ -257,6 +257,7 @@ bool G1ConcurrentRefineSweepState::swap_gc_threads_ct() { // For example in the rebuild remset process the marking threads write // marks into the card table, and that card table reference must be the // correct one. + SuspendibleThreadSet::synchronize_begin(); SuspendibleThreadSet::synchronize(); SuspendibleThreadSet::desynchronize(); }; diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index 5a5ec8863bd78..54d0fe6b4f651 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -28,8 +28,8 @@ #include "runtime/mutexLocker.hpp" #include "runtime/semaphore.hpp" -uint SuspendibleThreadSet::_nthreads = 0; -uint SuspendibleThreadSet::_nthreads_stopped = 0; +volatile uint SuspendibleThreadSet::_nthreads = 0; +volatile uint SuspendibleThreadSet::_nthreads_stopped = 0; volatile bool SuspendibleThreadSet::_suspend_all = false; double SuspendibleThreadSet::_suspend_all_start = 0.0; @@ -42,8 +42,8 @@ void SuspendibleThreadSet_init() { bool SuspendibleThreadSet::is_synchronized() { assert_lock_strong(STS_lock); - assert(_nthreads_stopped <= _nthreads, "invariant"); - return _nthreads_stopped == _nthreads; + assert(nthread_stoped() <= nthreads(), "invariant"); + return nthread_stoped() == nthreads(); } void SuspendibleThreadSet::join() { @@ -52,16 +52,16 @@ void SuspendibleThreadSet::join() { while (should_yield()) { ml.wait(); } - _nthreads++; + AtomicAccess::inc(&_nthreads); DEBUG_ONLY(Thread::current()->set_suspendible_thread();) } void SuspendibleThreadSet::leave() { assert(Thread::current()->is_suspendible_thread(), "Thread not joined"); MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); - assert(_nthreads > 0, "Invalid"); + assert(nthreads() > 0, "Invalid"); DEBUG_ONLY(Thread::current()->clear_suspendible_thread();) - _nthreads--; + AtomicAccess::dec(&_nthreads); if (should_yield() && is_synchronized()) { // This leave completes a request, so inform the requestor. _synchronize_wakeup->signal(); @@ -72,7 +72,7 @@ void SuspendibleThreadSet::yield_slow() { assert(Thread::current()->is_suspendible_thread(), "Must have joined"); MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); if (should_yield()) { - _nthreads_stopped++; + AtomicAccess::inc(&_nthreads_stopped); if (is_synchronized()) { if (ConcGCYieldTimeout > 0) { double now = os::elapsedTime(); @@ -84,8 +84,8 @@ void SuspendibleThreadSet::yield_slow() { while (should_yield()) { ml.wait(); } - assert(_nthreads_stopped > 0, "Invalid"); - _nthreads_stopped--; + assert(nthread_stoped() > 0, "Invalid"); + AtomicAccess::dec(&_nthreads_stopped); } } @@ -122,7 +122,7 @@ void SuspendibleThreadSet::synchronize() { // being signaled until we get back here again for some later // synchronize call. Hence, there is no need to re-check for // is_synchronized after the wait; it will always be true there. - log_trace(safepoint)("Waiting for %d GC threads to block", _nthreads - _nthreads_stopped); + log_trace(safepoint)("Waiting for %d GC threads to block", nthreads() - nthread_stoped()); _synchronize_wakeup->wait(); log_trace(safepoint)("All GC threads have been blocked"); diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp index 2e8e56b0c25de..645f5803130ae 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp @@ -41,11 +41,16 @@ class SuspendibleThreadSet : public AllStatic { friend class SuspendibleThreadSetLeaver; private: - static uint _nthreads; - static uint _nthreads_stopped; + static volatile uint _nthreads; + static volatile uint _nthreads_stopped; static volatile bool _suspend_all; static double _suspend_all_start; + + static bool nthreads() { return AtomicAccess::load(&_nthreads); } + + static bool nthread_stoped() { return AtomicAccess::load(&_nthreads_stopped); } + static bool is_synchronized(); // Add the current thread to the set. May block if a suspension is in progress. diff --git a/src/hotspot/share/gc/z/zGeneration.cpp b/src/hotspot/share/gc/z/zGeneration.cpp index d1680b6c33650..fd8f04d2cf8d4 100644 --- a/src/hotspot/share/gc/z/zGeneration.cpp +++ b/src/hotspot/share/gc/z/zGeneration.cpp @@ -1311,6 +1311,7 @@ class ZRendezvousGCThreads: public VM_Operation { void doit() { // Light weight "handshake" of the GC threads + SuspendibleThreadSet::synchronize_begin(); SuspendibleThreadSet::synchronize(); SuspendibleThreadSet::desynchronize(); }; From 82a1b1ce98d10d5225ba7a62d2edbde552a7d612 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Thu, 9 Oct 2025 15:34:18 -0700 Subject: [PATCH 04/11] Format --- src/hotspot/share/gc/shared/suspendibleThreadSet.cpp | 4 ++-- src/hotspot/share/gc/shared/suspendibleThreadSet.hpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index 54d0fe6b4f651..d80c2192faee5 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -28,8 +28,8 @@ #include "runtime/mutexLocker.hpp" #include "runtime/semaphore.hpp" -volatile uint SuspendibleThreadSet::_nthreads = 0; -volatile uint SuspendibleThreadSet::_nthreads_stopped = 0; +volatile uint SuspendibleThreadSet::_nthreads = 0; +volatile uint SuspendibleThreadSet::_nthreads_stopped = 0; volatile bool SuspendibleThreadSet::_suspend_all = false; double SuspendibleThreadSet::_suspend_all_start = 0.0; diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp index 645f5803130ae..21fcd5e970819 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp @@ -41,8 +41,8 @@ class SuspendibleThreadSet : public AllStatic { friend class SuspendibleThreadSetLeaver; private: - static volatile uint _nthreads; - static volatile uint _nthreads_stopped; + static volatile uint _nthreads; + static volatile uint _nthreads_stopped; static volatile bool _suspend_all; static double _suspend_all_start; From f1660923edc25d1d20b9665f5bcda37c2785f6ff Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Thu, 9 Oct 2025 15:58:12 -0700 Subject: [PATCH 05/11] Include logging/log.hpp --- src/hotspot/share/gc/shared/suspendibleThreadSet.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index d80c2192faee5..609948838e7c9 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -24,6 +24,7 @@ #include "gc/shared/gc_globals.hpp" #include "gc/shared/suspendibleThreadSet.hpp" +#include "logging/log.hpp" #include "runtime/javaThread.hpp" #include "runtime/mutexLocker.hpp" #include "runtime/semaphore.hpp" From 957c50a868dae38e7e3c95e826d219f30e4b9f60 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Thu, 9 Oct 2025 17:04:12 -0700 Subject: [PATCH 06/11] typo :( --- src/hotspot/share/gc/shared/suspendibleThreadSet.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp index 21fcd5e970819..318ff414001ad 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp @@ -47,9 +47,9 @@ class SuspendibleThreadSet : public AllStatic { static double _suspend_all_start; - static bool nthreads() { return AtomicAccess::load(&_nthreads); } + static uint nthreads() { return AtomicAccess::load(&_nthreads); } - static bool nthread_stoped() { return AtomicAccess::load(&_nthreads_stopped); } + static uint nthread_stoped() { return AtomicAccess::load(&_nthreads_stopped); } static bool is_synchronized(); From 94c0a552afff6884b0c88c18bbc9d6991155de51 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Thu, 9 Oct 2025 18:04:51 -0700 Subject: [PATCH 07/11] Revert changes to _nthreads/_nthreads_stopped --- .../share/gc/shared/suspendibleThreadSet.cpp | 23 ++++++++++--------- .../share/gc/shared/suspendibleThreadSet.hpp | 9 ++------ 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index 609948838e7c9..108356bb82dc1 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -29,8 +29,8 @@ #include "runtime/mutexLocker.hpp" #include "runtime/semaphore.hpp" -volatile uint SuspendibleThreadSet::_nthreads = 0; -volatile uint SuspendibleThreadSet::_nthreads_stopped = 0; +uint SuspendibleThreadSet::_nthreads = 0; +uint SuspendibleThreadSet::_nthreads_stopped = 0; volatile bool SuspendibleThreadSet::_suspend_all = false; double SuspendibleThreadSet::_suspend_all_start = 0.0; @@ -43,8 +43,8 @@ void SuspendibleThreadSet_init() { bool SuspendibleThreadSet::is_synchronized() { assert_lock_strong(STS_lock); - assert(nthread_stoped() <= nthreads(), "invariant"); - return nthread_stoped() == nthreads(); + assert(_nthreads_stopped <= _nthreads, "invariant"); + return _nthreads_stopped == _nthreads; } void SuspendibleThreadSet::join() { @@ -53,16 +53,16 @@ void SuspendibleThreadSet::join() { while (should_yield()) { ml.wait(); } - AtomicAccess::inc(&_nthreads); + _nthreads++; DEBUG_ONLY(Thread::current()->set_suspendible_thread();) } void SuspendibleThreadSet::leave() { assert(Thread::current()->is_suspendible_thread(), "Thread not joined"); MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); - assert(nthreads() > 0, "Invalid"); + assert(_nthreads > 0, "Invalid"); DEBUG_ONLY(Thread::current()->clear_suspendible_thread();) - AtomicAccess::dec(&_nthreads); + _nthreads--; if (should_yield() && is_synchronized()) { // This leave completes a request, so inform the requestor. _synchronize_wakeup->signal(); @@ -73,7 +73,7 @@ void SuspendibleThreadSet::yield_slow() { assert(Thread::current()->is_suspendible_thread(), "Must have joined"); MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); if (should_yield()) { - AtomicAccess::inc(&_nthreads_stopped); + _nthreads_stopped++; if (is_synchronized()) { if (ConcGCYieldTimeout > 0) { double now = os::elapsedTime(); @@ -85,8 +85,8 @@ void SuspendibleThreadSet::yield_slow() { while (should_yield()) { ml.wait(); } - assert(nthread_stoped() > 0, "Invalid"); - AtomicAccess::dec(&_nthreads_stopped); + assert(_nthreads_stopped > 0, "Invalid"); + _nthreads_stopped--; } } @@ -123,7 +123,7 @@ void SuspendibleThreadSet::synchronize() { // being signaled until we get back here again for some later // synchronize call. Hence, there is no need to re-check for // is_synchronized after the wait; it will always be true there. - log_trace(safepoint)("Waiting for %d GC threads to block", nthreads() - nthread_stoped()); + log_trace(safepoint)("Waiting for %d GC threads to block", _nthreads - _nthreads_stopped); _synchronize_wakeup->wait(); log_trace(safepoint)("All GC threads have been blocked"); @@ -138,6 +138,7 @@ void SuspendibleThreadSet::desynchronize() { MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); assert(should_yield(), "STS not synchronizing"); assert(is_synchronized(), "STS not synchronized"); + DEBUG_ONLY(OrderAccess::fence();) AtomicAccess::store(&_suspend_all, false); ml.notify_all(); } diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp index 318ff414001ad..2e8e56b0c25de 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp @@ -41,16 +41,11 @@ class SuspendibleThreadSet : public AllStatic { friend class SuspendibleThreadSetLeaver; private: - static volatile uint _nthreads; - static volatile uint _nthreads_stopped; + static uint _nthreads; + static uint _nthreads_stopped; static volatile bool _suspend_all; static double _suspend_all_start; - - static uint nthreads() { return AtomicAccess::load(&_nthreads); } - - static uint nthread_stoped() { return AtomicAccess::load(&_nthreads_stopped); } - static bool is_synchronized(); // Add the current thread to the set. May block if a suspension is in progress. From 2b39606483eeb430a8ba917f58385e59954252d3 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Fri, 10 Oct 2025 15:02:29 -0700 Subject: [PATCH 08/11] SuspendibleThreadSet::synchronize should never wait on semaphore _synchronize_wakeup if STS has synchronized at the beginning --- .../share/gc/shared/suspendibleThreadSet.cpp | 21 +++++++++++-------- .../share/gc/shared/suspendibleThreadSet.hpp | 1 + 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index 108356bb82dc1..efe7c52f0b769 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -31,6 +31,7 @@ uint SuspendibleThreadSet::_nthreads = 0; uint SuspendibleThreadSet::_nthreads_stopped = 0; +volatile bool SuspendibleThreadSet::_has_synchronized = false; volatile bool SuspendibleThreadSet::_suspend_all = false; double SuspendibleThreadSet::_suspend_all_start = 0.0; @@ -99,18 +100,18 @@ void SuspendibleThreadSet::synchronize_begin() { MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); assert(!should_yield(), "Only one at a time"); AtomicAccess::store(&_suspend_all, true); + if (is_synchronized()) { + AtomicAccess::store(&_has_synchronized, true); + } } } void SuspendibleThreadSet::synchronize() { assert(AtomicAccess::load(&_suspend_all), "synchronize must have begun."); - { - MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); - if (is_synchronized()) { - return; - } - } // Release lock before semaphore wait. - + // If STS has synchronized when synchronize begin, all suspendible threads will be waiting on STS_lock. + if (AtomicAccess::load(&_has_synchronized)) { + return; + } // Semaphore initial count is zero. To reach here, there must be at // least one not yielded thread in the set, e.g. is_synchronized() // was false before the lock was released. A thread in the set will @@ -123,9 +124,10 @@ void SuspendibleThreadSet::synchronize() { // being signaled until we get back here again for some later // synchronize call. Hence, there is no need to re-check for // is_synchronized after the wait; it will always be true there. - log_trace(safepoint)("Waiting for %d GC threads to block", _nthreads - _nthreads_stopped); + log_trace(safepoint)("Waiting for %d suspendible threads to block", _nthreads - _nthreads_stopped); _synchronize_wakeup->wait(); - log_trace(safepoint)("All GC threads have been blocked"); + AtomicAccess::store(&_has_synchronized, true); + log_trace(safepoint)("All suspendible threads have blocked"); #ifdef ASSERT MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); @@ -140,5 +142,6 @@ void SuspendibleThreadSet::desynchronize() { assert(is_synchronized(), "STS not synchronized"); DEBUG_ONLY(OrderAccess::fence();) AtomicAccess::store(&_suspend_all, false); + AtomicAccess::store(&_has_synchronized, false); ml.notify_all(); } diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp index 2e8e56b0c25de..cd0ff09fd7072 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.hpp @@ -43,6 +43,7 @@ class SuspendibleThreadSet : public AllStatic { private: static uint _nthreads; static uint _nthreads_stopped; + static volatile bool _has_synchronized; static volatile bool _suspend_all; static double _suspend_all_start; From 0f16621d88c715995f6709b9785a8ccf8618360b Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Fri, 10 Oct 2025 20:17:58 -0700 Subject: [PATCH 09/11] Bug fix --- src/hotspot/share/gc/shared/suspendibleThreadSet.cpp | 3 +-- src/hotspot/share/runtime/synchronizer.cpp | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp index efe7c52f0b769..0bba7012c052a 100644 --- a/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp +++ b/src/hotspot/share/gc/shared/suspendibleThreadSet.cpp @@ -140,8 +140,7 @@ void SuspendibleThreadSet::desynchronize() { MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag); assert(should_yield(), "STS not synchronizing"); assert(is_synchronized(), "STS not synchronized"); - DEBUG_ONLY(OrderAccess::fence();) - AtomicAccess::store(&_suspend_all, false); AtomicAccess::store(&_has_synchronized, false); + AtomicAccess::store(&_suspend_all, false); ml.notify_all(); } diff --git a/src/hotspot/share/runtime/synchronizer.cpp b/src/hotspot/share/runtime/synchronizer.cpp index e513c57fe069f..ebca44e60c4dc 100644 --- a/src/hotspot/share/runtime/synchronizer.cpp +++ b/src/hotspot/share/runtime/synchronizer.cpp @@ -1069,6 +1069,7 @@ class VM_RendezvousGCThreads : public VM_Operation { VMOp_Type type() const override { return VMOp_RendezvousGCThreads; } void doit() override { Universe::heap()->safepoint_synchronize_begin(); + Universe::heap()->safepoint_synchronize(); Universe::heap()->safepoint_synchronize_end(); }; }; From eefbe2fdfaa53bdad19c3944c9291241bc833e43 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Fri, 10 Oct 2025 22:11:45 -0700 Subject: [PATCH 10/11] Move synchronize_relocation to ZCollectedHeap::safepoint_synchronize --- src/hotspot/share/gc/z/zCollectedHeap.cpp | 4 ++-- src/hotspot/share/runtime/safepoint.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/gc/z/zCollectedHeap.cpp b/src/hotspot/share/gc/z/zCollectedHeap.cpp index 0ebde9afe8561..4e16c8097b5b2 100644 --- a/src/hotspot/share/gc/z/zCollectedHeap.cpp +++ b/src/hotspot/share/gc/z/zCollectedHeap.cpp @@ -333,12 +333,12 @@ bool ZCollectedHeap::contains_null(const oop* p) const { void ZCollectedHeap::safepoint_synchronize_begin() { StackWatermarkSet::safepoint_synchronize_begin(); - ZGeneration::young()->synchronize_relocation(); - ZGeneration::old()->synchronize_relocation(); SuspendibleThreadSet::synchronize_begin(); } void ZCollectedHeap::safepoint_synchronize() { + ZGeneration::young()->synchronize_relocation(); + ZGeneration::old()->synchronize_relocation(); SuspendibleThreadSet::synchronize(); } diff --git a/src/hotspot/share/runtime/safepoint.cpp b/src/hotspot/share/runtime/safepoint.cpp index e93f12db228e9..cea403badb909 100644 --- a/src/hotspot/share/runtime/safepoint.cpp +++ b/src/hotspot/share/runtime/safepoint.cpp @@ -370,10 +370,10 @@ void SafepointSynchronize::begin() { // Arms the safepoint, _current_jni_active_count and _waiting_to_block must be set before. log_trace(safepoint)("Arming safepoint using %s wait barrier", _wait_barrier->description()); arm_safepoint(); - + + Universe::heap()->safepoint_synchronize(); // Will spin until all threads are safe. int iterations = synchronize_threads(safepoint_limit_time, nof_threads, &initial_running); - Universe::heap()->safepoint_synchronize(); assert(_waiting_to_block == 0, "No thread should be running"); #ifndef PRODUCT From aa2bf8c51b7f4bd17e80ad2be2036b22f41b0e0f Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Fri, 10 Oct 2025 22:27:02 -0700 Subject: [PATCH 11/11] Format error --- src/hotspot/share/runtime/safepoint.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/runtime/safepoint.cpp b/src/hotspot/share/runtime/safepoint.cpp index cea403badb909..78990e4016041 100644 --- a/src/hotspot/share/runtime/safepoint.cpp +++ b/src/hotspot/share/runtime/safepoint.cpp @@ -370,7 +370,7 @@ void SafepointSynchronize::begin() { // Arms the safepoint, _current_jni_active_count and _waiting_to_block must be set before. log_trace(safepoint)("Arming safepoint using %s wait barrier", _wait_barrier->description()); arm_safepoint(); - + Universe::heap()->safepoint_synchronize(); // Will spin until all threads are safe. int iterations = synchronize_threads(safepoint_limit_time, nof_threads, &initial_running);