From 89af17016e19f16459564edc7873e00fc232eef2 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 4 Dec 2025 12:32:52 -0800 Subject: [PATCH 1/3] Expand scope of control lock so that it can't miss cancellation notifications --- .../shenandoahGenerationalControlThread.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp index ece4150f577e8..2d6abcf329d74 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp @@ -75,13 +75,15 @@ void ShenandoahGenerationalControlThread::run_service() { run_gc_cycle(request); } - // If the cycle was cancelled, continue the next iteration to deal with it. Otherwise, - // if there was no other cycle requested, cleanup and wait for the next request. - if (!_heap->cancelled_gc()) { + { + // If the cycle was cancelled, continue the next iteration to deal with it. Otherwise, + // if there was no other cycle requested, cleanup and wait for the next request. MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag); - if (_requested_gc_cause == GCCause::_no_gc) { - set_gc_mode(ml, none); - ml.wait(); + if (!_heap->cancelled_gc()) { + if (_requested_gc_cause == GCCause::_no_gc) { + set_gc_mode(ml, none); + ml.wait(); + } } } } From 1081f21e3a3b12f91c3930575a44bd40cda257ed Mon Sep 17 00:00:00 2001 From: William Kemper Date: Fri, 5 Dec 2025 08:15:34 -0800 Subject: [PATCH 2/3] Set requested gc cause under a lock when allocation fails --- .../shenandoahGenerationalControlThread.cpp | 37 +++++++++---------- .../shenandoahGenerationalControlThread.hpp | 7 +--- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp index 2d6abcf329d74..dfb28fd9ebb40 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp @@ -75,15 +75,13 @@ void ShenandoahGenerationalControlThread::run_service() { run_gc_cycle(request); } - { - // If the cycle was cancelled, continue the next iteration to deal with it. Otherwise, - // if there was no other cycle requested, cleanup and wait for the next request. + // If the cycle was cancelled, continue the next iteration to deal with it. Otherwise, + // if there was no other cycle requested, cleanup and wait for the next request. + if (!_heap->cancelled_gc()) { MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag); - if (!_heap->cancelled_gc()) { - if (_requested_gc_cause == GCCause::_no_gc) { - set_gc_mode(ml, none); - ml.wait(); - } + if (_requested_gc_cause == GCCause::_no_gc) { + set_gc_mode(ml, none); + ml.wait(); } } } @@ -98,8 +96,7 @@ void ShenandoahGenerationalControlThread::stop_service() { log_debug(gc, thread)("Stopping control thread"); MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag); _heap->cancel_gc(GCCause::_shenandoah_stop_vm); - _requested_gc_cause = GCCause::_shenandoah_stop_vm; - notify_cancellation(ml, GCCause::_shenandoah_stop_vm); + notify_control_thread(ml, GCCause::_shenandoah_stop_vm); // We can't wait here because it may interfere with the active cycle's ability // to reach a safepoint (this runs on a java thread). } @@ -142,7 +139,8 @@ void ShenandoahGenerationalControlThread::check_for_request(ShenandoahGCRequest& } ShenandoahGenerationalControlThread::GCMode ShenandoahGenerationalControlThread::prepare_for_allocation_failure_gc(ShenandoahGCRequest &request) { - + // Important: not all paths update the request.generation. This is intentional. + // A degenerated cycle must use the same generation carried over from the previous request. if (_degen_point == ShenandoahGC::_degenerated_unset) { _degen_point = ShenandoahGC::_degenerated_outside_cycle; request.generation = _heap->young_generation(); @@ -635,9 +633,7 @@ void ShenandoahGenerationalControlThread::service_stw_degenerated_cycle(const Sh void ShenandoahGenerationalControlThread::request_gc(GCCause::Cause cause) { if (ShenandoahCollectorPolicy::is_allocation_failure(cause)) { - // GC should already be cancelled. Here we are just notifying the control thread to - // wake up and handle the cancellation request, so we don't need to set _requested_gc_cause. - notify_cancellation(cause); + notify_control_thread(cause); } else if (ShenandoahCollectorPolicy::should_handle_requested_gc(cause)) { handle_requested_gc(cause); } @@ -663,7 +659,7 @@ bool ShenandoahGenerationalControlThread::request_concurrent_gc(ShenandoahGenera log_info(gc)("Preempting old generation mark to allow %s GC", generation->name()); while (gc_mode() == servicing_old) { ShenandoahHeap::heap()->cancel_gc(GCCause::_shenandoah_concurrent_gc); - notify_cancellation(ml, GCCause::_shenandoah_concurrent_gc); + notify_control_thread(ml, GCCause::_shenandoah_concurrent_gc); ml.wait(); } return true; @@ -703,14 +699,15 @@ void ShenandoahGenerationalControlThread::notify_control_thread(MonitorLocker& m ml.notify(); } -void ShenandoahGenerationalControlThread::notify_cancellation(GCCause::Cause cause) { +void ShenandoahGenerationalControlThread::notify_control_thread(GCCause::Cause cause) { MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag); - notify_cancellation(ml, cause); + notify_control_thread(ml, cause); } -void ShenandoahGenerationalControlThread::notify_cancellation(MonitorLocker& ml, GCCause::Cause cause) { - assert(_heap->cancelled_gc(), "GC should already be cancelled"); - log_debug(gc,thread)("Notify control (%s): %s", gc_mode_name(gc_mode()), GCCause::to_string(cause)); +void ShenandoahGenerationalControlThread::notify_control_thread(MonitorLocker& ml, GCCause::Cause cause) { + assert(_control_lock.is_locked(), "Request lock must be held here"); + log_debug(gc, thread)("Notify control (%s): %s", gc_mode_name(gc_mode()), GCCause::to_string(cause)); + _requested_gc_cause = cause; ml.notify(); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp index b7dbedd5e8461..1544f470c6479 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp @@ -139,11 +139,8 @@ class ShenandoahGenerationalControlThread: public ShenandoahController { // The overloaded variant should be used when the _control_lock is already held. void notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation); void notify_control_thread(MonitorLocker& ml, GCCause::Cause cause, ShenandoahGeneration* generation); - - // Notifies the control thread, but does not update the requested cause or generation. - // The overloaded variant should be used when the _control_lock is already held. - void notify_cancellation(GCCause::Cause cause); - void notify_cancellation(MonitorLocker& ml, GCCause::Cause cause); + void notify_control_thread(GCCause::Cause cause); + void notify_control_thread(MonitorLocker& ml, GCCause::Cause cause); // Configure the heap to age objects and regions if the aging period has elapsed. void maybe_set_aging_cycle(); From baed458a6f25366e1119848a150e71e60f87de28 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Fri, 12 Dec 2025 16:33:18 -0800 Subject: [PATCH 3/3] Improve comment --- .../gc/shenandoah/shenandoahGenerationalControlThread.hpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp index 1544f470c6479..13e69d2526870 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp @@ -135,12 +135,12 @@ class ShenandoahGenerationalControlThread: public ShenandoahController { // Return printable name for the given gc mode. static const char* gc_mode_name(GCMode mode); - // Takes the request lock and updates the requested cause and generation, then notifies the control thread. - // The overloaded variant should be used when the _control_lock is already held. - void notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation); - void notify_control_thread(MonitorLocker& ml, GCCause::Cause cause, ShenandoahGeneration* generation); + // These notify the control thread after updating _requested_gc_cause and (optionally) _requested_generation. + // Updating the requested generation is not necessary for allocation failures nor when stopping the thread. void notify_control_thread(GCCause::Cause cause); void notify_control_thread(MonitorLocker& ml, GCCause::Cause cause); + void notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation); + void notify_control_thread(MonitorLocker& ml, GCCause::Cause cause, ShenandoahGeneration* generation); // Configure the heap to age objects and regions if the aging period has elapsed. void maybe_set_aging_cycle();