From 8493d546ab80bd84d4d432a6cfb5d007583cecd3 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 16 Oct 2025 15:27:33 -0700 Subject: [PATCH 01/19] Filter out young pointers from thread local SATB buffers if young is not being marked --- .../shenandoah/shenandoahSATBMarkQueueSet.cpp | 35 ++++++++++++++++--- .../shenandoah/shenandoahSATBMarkQueueSet.hpp | 8 ++--- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp index 547ebb1a2296f..af0a2e519cef4 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp @@ -39,16 +39,43 @@ class ShenandoahSATBMarkQueueFilterFn { ShenandoahHeap* const _heap; public: - ShenandoahSATBMarkQueueFilterFn(ShenandoahHeap* heap) : _heap(heap) {} + explicit ShenandoahSATBMarkQueueFilterFn(ShenandoahHeap* heap) : _heap(heap) {} - // Return true if entry should be filtered out (removed), false if - // it should be retained. + // Return true if entry should be filtered out (removed), false if it should be retained. bool operator()(const void* entry) const { return !_heap->requires_marking(entry); } }; +class ShenandoahSATBOldMarkQueueFilterFn { + ShenandoahHeap* const _heap; + size_t _filtered_young; + +public: + explicit ShenandoahSATBOldMarkQueueFilterFn(ShenandoahHeap* heap) : _heap(heap), _filtered_young(0) {} + ~ShenandoahSATBOldMarkQueueFilterFn() { + if (_filtered_young > 0) { + log_debug(gc, stats)("Filtered %zu young pointers during old mark", _filtered_young); + } + } + + // Return true if entry should be filtered out (removed), false if it should be retained. + bool operator()(const void* entry) const { + assert(_heap->is_concurrent_old_mark_in_progress(), "Should only use this when old marking is in progress"); + assert(!_heap->is_concurrent_young_mark_in_progress(), "Should only use this when young marking is not in progress"); + if (!_heap->is_in_old(entry)) { + const_cast(this)->_filtered_young++; + return true; + } + return !_heap->requires_marking(entry); + } +}; + void ShenandoahSATBMarkQueueSet::filter(SATBMarkQueue& queue) { ShenandoahHeap* heap = ShenandoahHeap::heap(); - apply_filter(ShenandoahSATBMarkQueueFilterFn(heap), queue); + if (heap->is_concurrent_old_mark_in_progress() && !heap->is_concurrent_young_mark_in_progress()) { + apply_filter(ShenandoahSATBOldMarkQueueFilterFn(heap), queue); + } else { + apply_filter(ShenandoahSATBMarkQueueFilterFn(heap), queue); + } } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp index c30d25076762b..42069598a739c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp @@ -27,15 +27,13 @@ #include "gc/shared/bufferNode.hpp" #include "gc/shared/satbMarkQueue.hpp" -#include "runtime/javaThread.hpp" -#include "runtime/mutex.hpp" class ShenandoahSATBMarkQueueSet : public SATBMarkQueueSet { public: - ShenandoahSATBMarkQueueSet(BufferNode::Allocator* allocator); + explicit ShenandoahSATBMarkQueueSet(BufferNode::Allocator* allocator); - virtual SATBMarkQueue& satb_queue_for_thread(Thread* const t) const; - virtual void filter(SATBMarkQueue& queue); + SATBMarkQueue& satb_queue_for_thread(Thread* const t) const override; + void filter(SATBMarkQueue& queue) override; }; #endif // SHARE_GC_SHENANDOAH_SHENANDOAHSATBMARKQUEUESET_HPP From ac36718fc8bc56316ea1cd4f690790655184d6b5 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 16 Oct 2025 15:29:54 -0700 Subject: [PATCH 02/19] Temporarily increase log level --- src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp index af0a2e519cef4..3947094b31ff2 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp @@ -55,7 +55,7 @@ class ShenandoahSATBOldMarkQueueFilterFn { explicit ShenandoahSATBOldMarkQueueFilterFn(ShenandoahHeap* heap) : _heap(heap), _filtered_young(0) {} ~ShenandoahSATBOldMarkQueueFilterFn() { if (_filtered_young > 0) { - log_debug(gc, stats)("Filtered %zu young pointers during old mark", _filtered_young); + log_info(gc, stats)("Filtered %zu young pointers during old mark", _filtered_young); } } From bdbd8e3bddfa79071567db42d29f013415f8c7ee Mon Sep 17 00:00:00 2001 From: William Kemper Date: Fri, 17 Oct 2025 14:49:36 -0700 Subject: [PATCH 03/19] Remove instrumentation --- .../gc/shenandoah/shenandoahSATBMarkQueueSet.cpp | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp index 3947094b31ff2..7a403db7f1be6 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp @@ -49,25 +49,15 @@ class ShenandoahSATBMarkQueueFilterFn { class ShenandoahSATBOldMarkQueueFilterFn { ShenandoahHeap* const _heap; - size_t _filtered_young; public: - explicit ShenandoahSATBOldMarkQueueFilterFn(ShenandoahHeap* heap) : _heap(heap), _filtered_young(0) {} - ~ShenandoahSATBOldMarkQueueFilterFn() { - if (_filtered_young > 0) { - log_info(gc, stats)("Filtered %zu young pointers during old mark", _filtered_young); - } - } + explicit ShenandoahSATBOldMarkQueueFilterFn(ShenandoahHeap* heap) : _heap(heap) {} // Return true if entry should be filtered out (removed), false if it should be retained. bool operator()(const void* entry) const { assert(_heap->is_concurrent_old_mark_in_progress(), "Should only use this when old marking is in progress"); assert(!_heap->is_concurrent_young_mark_in_progress(), "Should only use this when young marking is not in progress"); - if (!_heap->is_in_old(entry)) { - const_cast(this)->_filtered_young++; - return true; - } - return !_heap->requires_marking(entry); + return !_heap->requires_marking(entry) || !_heap->is_in_old(entry); } }; From 039a08d22c43fde604344157c5052fce977fa1c2 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Mon, 20 Oct 2025 09:54:36 -0700 Subject: [PATCH 04/19] Comment out all calls to transfer old pointers out of SATB --- src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index 456b9fe6d3c85..5655ff43ec98c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -292,7 +292,7 @@ bool ShenandoahConcurrentGC::complete_abbreviated_cycle() { ShenandoahSATBMarkQueueSet& satb_queues = ShenandoahBarrierSet::satb_mark_queue_set(); ShenandoahFlushSATBHandshakeClosure complete_thread_local_satb_buffers(satb_queues); heap->concurrent_final_roots(&complete_thread_local_satb_buffers); - heap->old_generation()->concurrent_transfer_pointers_from_satb(); + // heap->old_generation()->concurrent_transfer_pointers_from_satb(); } return true; } @@ -692,7 +692,7 @@ void ShenandoahConcurrentGC::op_init_mark() { // old generation mark queue. Any pointers in a young region will be // abandoned. ShenandoahGCPhase phase(ShenandoahPhaseTimings::init_transfer_satb); - heap->old_generation()->transfer_pointers_from_satb(); + // heap->old_generation()->transfer_pointers_from_satb(); } { // After we swap card table below, the write-table is all clean, and the read table holds @@ -1189,7 +1189,7 @@ void ShenandoahConcurrentGC::op_final_update_refs() { // with no live objects cannot have been written to and so cannot have entries in the SATB // buffers. ShenandoahGCPhase phase(ShenandoahPhaseTimings::final_update_refs_transfer_satb); - heap->old_generation()->transfer_pointers_from_satb(); + // heap->old_generation()->transfer_pointers_from_satb(); // Aging_cycle is only relevant during evacuation cycle for individual objects and during final mark for // entire regions. Both of these relevant operations occur before final update refs. From bbf207c2253b7899fd5d65ab582dfd7341a90fb9 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Mon, 20 Oct 2025 17:34:47 -0700 Subject: [PATCH 05/19] Need to flush thread local SATB buffers before young marking Otherwise, the filter won't filter out young (possibly garbage) pointers. I don't think this needs to happen on a safepoint. We could probably even do it as part of pausing old GC. --- src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index 5655ff43ec98c..4548ee3d5256d 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -692,7 +692,7 @@ void ShenandoahConcurrentGC::op_init_mark() { // old generation mark queue. Any pointers in a young region will be // abandoned. ShenandoahGCPhase phase(ShenandoahPhaseTimings::init_transfer_satb); - // heap->old_generation()->transfer_pointers_from_satb(); + heap->old_generation()->transfer_pointers_from_satb(); } { // After we swap card table below, the write-table is all clean, and the read table holds From 1301df42aacee77654b524f2fa0998567eca5635 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Tue, 21 Oct 2025 10:33:15 -0700 Subject: [PATCH 06/19] Avoid checking gc-state every time we filter a SATB buffer --- .../share/gc/shenandoah/shenandoahConcurrentGC.cpp | 9 ++++++++- .../share/gc/shenandoah/shenandoahDegeneratedGC.cpp | 2 ++ .../share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp | 4 ++-- .../share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp | 10 ++++++++++ 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index 4548ee3d5256d..efcdd5c96e125 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -689,9 +689,11 @@ void ShenandoahConcurrentGC::op_init_mark() { heap->old_generation()->cancel_gc(); } else if (heap->is_concurrent_old_mark_in_progress()) { // Purge the SATB buffers, transferring any valid, old pointers to the - // old generation mark queue. Any pointers in a young region will be + // old generation mark queue. Any pointers not in an old region will be // abandoned. ShenandoahGCPhase phase(ShenandoahPhaseTimings::init_transfer_satb); + assert(ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), + "Should be filtering pointers outside of old during old marking"); heap->old_generation()->transfer_pointers_from_satb(); } { @@ -714,6 +716,7 @@ void ShenandoahConcurrentGC::op_init_mark() { } _generation->set_concurrent_mark_in_progress(true); + ShenandoahBarrierSet::satb_mark_queue_set().set_filter_out_young(false); start_mark(); @@ -814,6 +817,10 @@ void ShenandoahConcurrentGC::op_final_mark() { } } + if (heap->is_concurrent_old_mark_in_progress()) { + ShenandoahBarrierSet::satb_mark_queue_set().set_filter_out_young(true); + } + { ShenandoahTimingsTracker timing(ShenandoahPhaseTimings::final_mark_propagate_gc_state); heap->propagate_gc_state_to_all_threads(); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp index b918bf67b34b7..6af33c7d9a12a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp @@ -186,6 +186,8 @@ void ShenandoahDegenGC::op_degenerated() { op_mark(); case _degenerated_mark: + assert(!ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), + "Should not be filtering out young pointers when concurrent mark degenerates"); // No fallthrough. Continue mark, handed over from concurrent mark if // concurrent mark has yet completed if (_degen_point == ShenandoahDegenPoint::_degenerated_mark && diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp index 7a403db7f1be6..c2ed575b43838 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp @@ -28,7 +28,7 @@ #include "gc/shenandoah/shenandoahThreadLocalData.hpp" ShenandoahSATBMarkQueueSet::ShenandoahSATBMarkQueueSet(BufferNode::Allocator* allocator) : - SATBMarkQueueSet(allocator) + SATBMarkQueueSet(allocator), _filter_out_young(false) {} SATBMarkQueue& ShenandoahSATBMarkQueueSet::satb_queue_for_thread(Thread* const t) const { @@ -63,7 +63,7 @@ class ShenandoahSATBOldMarkQueueFilterFn { void ShenandoahSATBMarkQueueSet::filter(SATBMarkQueue& queue) { ShenandoahHeap* heap = ShenandoahHeap::heap(); - if (heap->is_concurrent_old_mark_in_progress() && !heap->is_concurrent_young_mark_in_progress()) { + if (_filter_out_young) { apply_filter(ShenandoahSATBOldMarkQueueFilterFn(heap), queue); } else { apply_filter(ShenandoahSATBMarkQueueFilterFn(heap), queue); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp index 42069598a739c..dd4cdfebc176b 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.hpp @@ -29,11 +29,21 @@ #include "gc/shared/satbMarkQueue.hpp" class ShenandoahSATBMarkQueueSet : public SATBMarkQueueSet { +private: + bool _filter_out_young; + public: explicit ShenandoahSATBMarkQueueSet(BufferNode::Allocator* allocator); SATBMarkQueue& satb_queue_for_thread(Thread* const t) const override; void filter(SATBMarkQueue& queue) override; + void set_filter_out_young(bool filter_out_young) { + _filter_out_young = filter_out_young; + } + + bool get_filter_out_young() const { + return _filter_out_young; + } }; #endif // SHARE_GC_SHENANDOAH_SHENANDOAHSATBMARKQUEUESET_HPP From eb650039ffd1215264ceeb5aa7183d75aaf9f260 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Tue, 21 Oct 2025 10:35:00 -0700 Subject: [PATCH 07/19] Stop processing completed SATB buffers, they should no longer have bad pointers --- .../gc/shenandoah/shenandoahConcurrentGC.cpp | 21 +------ .../gc/shenandoah/shenandoahOldGeneration.cpp | 56 +++---------------- .../gc/shenandoah/shenandoahOldGeneration.hpp | 1 - 3 files changed, 10 insertions(+), 68 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index efcdd5c96e125..2685647b4b73c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -279,21 +279,6 @@ bool ShenandoahConcurrentGC::complete_abbreviated_cycle() { heap->update_region_ages(_generation->complete_marking_context()); } - if (!heap->is_concurrent_old_mark_in_progress()) { - heap->concurrent_final_roots(); - } else { - // Since the cycle was shortened for having enough immediate garbage, this will be - // the last phase before concurrent marking of old resumes. We must be sure - // that old mark threads don't see any pointers to garbage in the SATB queues. Even - // though nothing was evacuated, overwriting unreachable weak roots with null may still - // put pointers to regions that become trash in the SATB queues. The following will - // piggyback flushing the thread local SATB queues on the same handshake that propagates - // the gc state change. - ShenandoahSATBMarkQueueSet& satb_queues = ShenandoahBarrierSet::satb_mark_queue_set(); - ShenandoahFlushSATBHandshakeClosure complete_thread_local_satb_buffers(satb_queues); - heap->concurrent_final_roots(&complete_thread_local_satb_buffers); - // heap->old_generation()->concurrent_transfer_pointers_from_satb(); - } return true; } @@ -1232,13 +1217,13 @@ bool ShenandoahConcurrentGC::entry_final_roots() { ShenandoahWorkerPolicy::calc_workers_for_conc_evac(), msg); - if (!heap->mode()->is_generational()) { - heap->concurrent_final_roots(); - } else { + if (heap->mode()->is_generational()) { if (!complete_abbreviated_cycle()) { return false; } } + + heap->concurrent_final_roots(); return true; } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp index ac3107eb396ec..e0e9ce0713923 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp @@ -116,40 +116,13 @@ class ShenandoahPurgeSATBTask : public WorkerTask { ShenandoahFlushAllSATB flusher(satb_queues); Threads::possibly_parallel_threads_do(true /* is_par */, &flusher); - ShenandoahObjToScanQueue* mark_queue = _mark_queues->queue(worker_id); - ShenandoahProcessOldSATB processor(mark_queue); - while (satb_queues.apply_closure_to_completed_buffer(&processor)) {} - - AtomicAccess::add(&_trashed_oops, processor.trashed_oops()); - } -}; - -class ShenandoahTransferOldSATBTask : public WorkerTask { - ShenandoahSATBMarkQueueSet& _satb_queues; - ShenandoahObjToScanQueueSet* _mark_queues; - // Keep track of the number of oops that are not transferred to mark queues. - // This is volatile because workers update it, but the control thread reads it. - volatile size_t _trashed_oops; - -public: - explicit ShenandoahTransferOldSATBTask(ShenandoahSATBMarkQueueSet& satb_queues, ShenandoahObjToScanQueueSet* mark_queues) : - WorkerTask("Transfer SATB"), - _satb_queues(satb_queues), - _mark_queues(mark_queues), - _trashed_oops(0) {} - - ~ShenandoahTransferOldSATBTask() { - if (_trashed_oops > 0) { - log_debug(gc)("Purged %zu oops from old generation SATB buffers", _trashed_oops); - } - } - - void work(uint worker_id) override { - ShenandoahObjToScanQueue* mark_queue = _mark_queues->queue(worker_id); - ShenandoahProcessOldSATB processor(mark_queue); - while (_satb_queues.apply_closure_to_completed_buffer(&processor)) {} - - AtomicAccess::add(&_trashed_oops, processor.trashed_oops()); + // Now that we are filtering _before_ the buffers are complete, we should no + // longer need to filter when transferring completed buffers to the mark queues. + // Handling completed buffers is a normal part of concurrent mark and should just work (TM). + // ShenandoahObjToScanQueue* mark_queue = _mark_queues->queue(worker_id); + // ShenandoahProcessOldSATB processor(mark_queue); + // while (satb_queues.apply_closure_to_completed_buffer(&processor)) {} + // AtomicAccess::add(&_trashed_oops, processor.trashed_oops()); } }; @@ -452,21 +425,6 @@ bool ShenandoahOldGeneration::coalesce_and_fill() { } } -void ShenandoahOldGeneration::concurrent_transfer_pointers_from_satb() const { - const ShenandoahHeap* heap = ShenandoahHeap::heap(); - assert(heap->is_concurrent_old_mark_in_progress(), "Only necessary during old marking."); - log_debug(gc)("Transfer SATB buffers"); - - // Step 1. All threads need to 'complete' partially filled, thread local SATB buffers. This - // is accomplished in ShenandoahConcurrentGC::complete_abbreviated_cycle using a Handshake - // operation. - // Step 2. Use worker threads to transfer oops from old, active regions in the completed - // SATB buffers to old generation mark queues. - ShenandoahSATBMarkQueueSet& satb_queues = ShenandoahBarrierSet::satb_mark_queue_set(); - ShenandoahTransferOldSATBTask transfer_task(satb_queues, task_queues()); - heap->workers()->run_task(&transfer_task); -} - void ShenandoahOldGeneration::transfer_pointers_from_satb() const { const ShenandoahHeap* heap = ShenandoahHeap::heap(); assert(heap->is_concurrent_old_mark_in_progress(), "Only necessary during old marking."); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp index abc865c31cd1e..1199b31a7bea2 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp @@ -233,7 +233,6 @@ class ShenandoahOldGeneration : public ShenandoahGeneration { // object at the barrier, but we reject this approach because it is likely // the performance impact would be too severe. void transfer_pointers_from_satb() const; - void concurrent_transfer_pointers_from_satb() const; // True if there are old regions waiting to be selected for a mixed collection bool has_unprocessed_collection_candidates(); From 05aa120c41e7a1de4a4533f649c04c605891b039 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Tue, 21 Oct 2025 15:42:34 -0700 Subject: [PATCH 08/19] Manage satb filter at safepoint exit --- src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp | 5 ----- .../share/gc/shenandoah/shenandoahDegeneratedGC.cpp | 7 +++---- src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp | 3 +++ 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index 2685647b4b73c..cbefef1c6da7c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -701,7 +701,6 @@ void ShenandoahConcurrentGC::op_init_mark() { } _generation->set_concurrent_mark_in_progress(true); - ShenandoahBarrierSet::satb_mark_queue_set().set_filter_out_young(false); start_mark(); @@ -802,10 +801,6 @@ void ShenandoahConcurrentGC::op_final_mark() { } } - if (heap->is_concurrent_old_mark_in_progress()) { - ShenandoahBarrierSet::satb_mark_queue_set().set_filter_out_young(true); - } - { ShenandoahTimingsTracker timing(ShenandoahPhaseTimings::final_mark_propagate_gc_state); heap->propagate_gc_state_to_all_threads(); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp index 6af33c7d9a12a..8032dd13d76d7 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp @@ -186,12 +186,11 @@ void ShenandoahDegenGC::op_degenerated() { op_mark(); case _degenerated_mark: - assert(!ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), - "Should not be filtering out young pointers when concurrent mark degenerates"); // No fallthrough. Continue mark, handed over from concurrent mark if // concurrent mark has yet completed - if (_degen_point == ShenandoahDegenPoint::_degenerated_mark && - heap->is_concurrent_mark_in_progress()) { + if (_degen_point == ShenandoahDegenPoint::_degenerated_mark && heap->is_concurrent_mark_in_progress()) { + assert(!ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), + "Should not be filtering out young pointers when concurrent mark degenerates"); op_finish_mark(); } assert(!heap->cancelled_gc(), "STW mark can not OOM"); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index b2fd32d2fd0b9..6df21e9c3f2bb 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -2088,6 +2088,9 @@ void ShenandoahHeap::prepare_update_heap_references() { void ShenandoahHeap::propagate_gc_state_to_all_threads() { assert(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "Must be at Shenandoah safepoint"); if (_gc_state_changed) { + ShenandoahBarrierSet::satb_mark_queue_set().set_filter_out_young( + is_concurrent_old_mark_in_progress() && !is_concurrent_young_mark_in_progress() + ); ShenandoahGCStatePropagatorHandshakeClosure propagator(_gc_state.raw_value()); Threads::threads_do(&propagator); _gc_state_changed = false; From 004a9540a59167e69435248636336f33b35bf0f6 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Wed, 22 Oct 2025 14:09:24 -0700 Subject: [PATCH 09/19] Remove unused code --- .../gc/shenandoah/shenandoahOldGeneration.cpp | 59 +------------------ 1 file changed, 2 insertions(+), 57 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp index e0e9ce0713923..1de256383b5f3 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp @@ -57,72 +57,17 @@ class ShenandoahFlushAllSATB : public ThreadClosure { } }; -class ShenandoahProcessOldSATB : public SATBBufferClosure { -private: - ShenandoahObjToScanQueue* _queue; - ShenandoahHeap* _heap; - ShenandoahMarkingContext* const _mark_context; - size_t _trashed_oops; - -public: - explicit ShenandoahProcessOldSATB(ShenandoahObjToScanQueue* q) : - _queue(q), - _heap(ShenandoahHeap::heap()), - _mark_context(_heap->marking_context()), - _trashed_oops(0) {} - - void do_buffer(void** buffer, size_t size) override { - assert(size == 0 || !_heap->has_forwarded_objects() || _heap->is_concurrent_old_mark_in_progress(), "Forwarded objects are not expected here"); - for (size_t i = 0; i < size; ++i) { - oop *p = (oop *) &buffer[i]; - ShenandoahHeapRegion* region = _heap->heap_region_containing(*p); - if (region->is_old() && region->is_active()) { - ShenandoahMark::mark_through_ref(p, _queue, nullptr, _mark_context, false); - } else { - _trashed_oops++; - } - } - } - - size_t trashed_oops() const { - return _trashed_oops; - } -}; - class ShenandoahPurgeSATBTask : public WorkerTask { -private: - ShenandoahObjToScanQueueSet* _mark_queues; - // Keep track of the number of oops that are not transferred to mark queues. - // This is volatile because workers update it, but the vm thread reads it. - volatile size_t _trashed_oops; - public: - explicit ShenandoahPurgeSATBTask(ShenandoahObjToScanQueueSet* queues) : - WorkerTask("Purge SATB"), - _mark_queues(queues), - _trashed_oops(0) { + explicit ShenandoahPurgeSATBTask() : WorkerTask("Purge SATB") { Threads::change_thread_claim_token(); } - ~ShenandoahPurgeSATBTask() { - if (_trashed_oops > 0) { - log_debug(gc)("Purged %zu oops from old generation SATB buffers", _trashed_oops); - } - } - void work(uint worker_id) override { ShenandoahParallelWorkerSession worker_session(worker_id); ShenandoahSATBMarkQueueSet &satb_queues = ShenandoahBarrierSet::satb_mark_queue_set(); ShenandoahFlushAllSATB flusher(satb_queues); Threads::possibly_parallel_threads_do(true /* is_par */, &flusher); - - // Now that we are filtering _before_ the buffers are complete, we should no - // longer need to filter when transferring completed buffers to the mark queues. - // Handling completed buffers is a normal part of concurrent mark and should just work (TM). - // ShenandoahObjToScanQueue* mark_queue = _mark_queues->queue(worker_id); - // ShenandoahProcessOldSATB processor(mark_queue); - // while (satb_queues.apply_closure_to_completed_buffer(&processor)) {} - // AtomicAccess::add(&_trashed_oops, processor.trashed_oops()); } }; @@ -429,7 +374,7 @@ void ShenandoahOldGeneration::transfer_pointers_from_satb() const { const ShenandoahHeap* heap = ShenandoahHeap::heap(); assert(heap->is_concurrent_old_mark_in_progress(), "Only necessary during old marking."); log_debug(gc)("Transfer SATB buffers"); - ShenandoahPurgeSATBTask purge_satb_task(task_queues()); + ShenandoahPurgeSATBTask purge_satb_task; heap->workers()->run_task(&purge_satb_task); } From 03a4dad1aeada861b2267a730fad2eb350668f70 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 23 Oct 2025 10:36:45 -0700 Subject: [PATCH 10/19] Flush old satb after update references This should be the last and only time it is necessary --- .../share/gc/shenandoah/shenandoahConcurrentGC.cpp | 10 +++++----- .../share/gc/shenandoah/shenandoahOldGeneration.cpp | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index cbefef1c6da7c..ea686562316c3 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -676,10 +676,8 @@ void ShenandoahConcurrentGC::op_init_mark() { // Purge the SATB buffers, transferring any valid, old pointers to the // old generation mark queue. Any pointers not in an old region will be // abandoned. - ShenandoahGCPhase phase(ShenandoahPhaseTimings::init_transfer_satb); - assert(ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), - "Should be filtering pointers outside of old during old marking"); - heap->old_generation()->transfer_pointers_from_satb(); + // ShenandoahGCPhase phase(ShenandoahPhaseTimings::init_transfer_satb); + // heap->old_generation()->transfer_pointers_from_satb(); } { // After we swap card table below, the write-table is all clean, and the read table holds @@ -1175,8 +1173,10 @@ void ShenandoahConcurrentGC::op_final_update_refs() { // We are not concerned about skipping this step in abbreviated cycles because regions // with no live objects cannot have been written to and so cannot have entries in the SATB // buffers. + // + // After update refs, it is no longer possible for a mutator to overwrite a pointer into the collection set ShenandoahGCPhase phase(ShenandoahPhaseTimings::final_update_refs_transfer_satb); - // heap->old_generation()->transfer_pointers_from_satb(); + heap->old_generation()->transfer_pointers_from_satb(); // Aging_cycle is only relevant during evacuation cycle for individual objects and during final mark for // entire regions. Both of these relevant operations occur before final update refs. diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp index 1de256383b5f3..af0f55885b1b2 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp @@ -373,6 +373,8 @@ bool ShenandoahOldGeneration::coalesce_and_fill() { void ShenandoahOldGeneration::transfer_pointers_from_satb() const { const ShenandoahHeap* heap = ShenandoahHeap::heap(); assert(heap->is_concurrent_old_mark_in_progress(), "Only necessary during old marking."); + assert(ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), + "Should be filtering pointers outside of old during old marking"); log_debug(gc)("Transfer SATB buffers"); ShenandoahPurgeSATBTask purge_satb_task; heap->workers()->run_task(&purge_satb_task); From e4f4f8ac30f9174b9845dcab08ebd04b2a56ac85 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 23 Oct 2025 12:04:47 -0700 Subject: [PATCH 11/19] Try piggybacking satb flush on update roots --- .../gc/shenandoah/shenandoahClosures.hpp | 12 ++++++- .../shenandoah/shenandoahClosures.inline.hpp | 5 +++ .../gc/shenandoah/shenandoahConcurrentGC.cpp | 32 ++++++++++++++++--- .../gc/shenandoah/shenandoahOldGeneration.cpp | 14 -------- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp b/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp index 0c223ee3128be..92a89fc1be991 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp @@ -24,7 +24,6 @@ #ifndef SHARE_GC_SHENANDOAH_SHENANDOAHCLOSURES_HPP #define SHARE_GC_SHENANDOAH_SHENANDOAHCLOSURES_HPP -#include "code/nmethod.hpp" #include "gc/shared/stringdedup/stringDedup.hpp" #include "gc/shenandoah/shenandoahGenerationType.hpp" #include "gc/shenandoah/shenandoahTaskqueue.hpp" @@ -230,6 +229,17 @@ class ShenandoahConcUpdateRefsClosure : public ShenandoahUpdateRefsSuperClosure }; +class ShenandoahFlushAllSATB : public ThreadClosure { +private: + SATBMarkQueueSet& _satb_qset; + +public: + explicit ShenandoahFlushAllSATB(SATBMarkQueueSet& satb_qset) : _satb_qset(satb_qset) {} + + inline void do_thread(Thread* thread) override; +}; + + // // ========= Utilities // diff --git a/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp index 427eaad26ce21..36aa55e8c957d 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp @@ -268,6 +268,11 @@ void ShenandoahAssertNotForwardedClosure::do_oop_work(T* p) { } } +inline void ShenandoahFlushAllSATB::do_thread(Thread* thread) { + // Transfer any partial buffer to the qset for completed buffer processing. + _satb_qset.flush_queue(ShenandoahThreadLocalData::satb_mark_queue(thread)); +} + void ShenandoahAssertNotForwardedClosure::do_oop(narrowOop* p) { do_oop_work(p); } void ShenandoahAssertNotForwardedClosure::do_oop(oop* p) { do_oop_work(p); } #endif diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index ea686562316c3..b97a4612d3cfa 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -1113,7 +1113,7 @@ class ShenandoahUpdateThreadHandshakeClosure : public HandshakeClosure { ShenandoahNonConcUpdateRefsClosure _cl; public: ShenandoahUpdateThreadHandshakeClosure(); - void do_thread(Thread* thread); + void do_thread(Thread* thread) override; }; ShenandoahUpdateThreadHandshakeClosure::ShenandoahUpdateThreadHandshakeClosure() : @@ -1128,9 +1128,31 @@ void ShenandoahUpdateThreadHandshakeClosure::do_thread(Thread* thread) { } } +class ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers final : public HandshakeClosure { + ShenandoahUpdateThreadHandshakeClosure _update_roots; + ShenandoahFlushAllSATB _flush_all_satb; + +public: + ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers() : + HandshakeClosure("Shenandoah Update Thread Roots and Flush SATB"), + _flush_all_satb(ShenandoahBarrierSet::satb_mark_queue_set()) { + } + + void do_thread(Thread* thread) override { + _update_roots.do_thread(thread); + _flush_all_satb.do_thread(thread); + } +}; + void ShenandoahConcurrentGC::op_update_thread_roots() { - ShenandoahUpdateThreadHandshakeClosure cl; - Handshake::execute(&cl); + ShenandoahHeap* const heap = ShenandoahHeap::heap(); + if (heap->is_concurrent_old_mark_in_progress()) { + ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers cl; + Handshake::execute(&cl); + } else { + ShenandoahUpdateThreadHandshakeClosure cl; + Handshake::execute(&cl); + } } void ShenandoahConcurrentGC::op_final_update_refs() { @@ -1175,8 +1197,8 @@ void ShenandoahConcurrentGC::op_final_update_refs() { // buffers. // // After update refs, it is no longer possible for a mutator to overwrite a pointer into the collection set - ShenandoahGCPhase phase(ShenandoahPhaseTimings::final_update_refs_transfer_satb); - heap->old_generation()->transfer_pointers_from_satb(); + // ShenandoahGCPhase phase(ShenandoahPhaseTimings::final_update_refs_transfer_satb); + // heap->old_generation()->transfer_pointers_from_satb(); // Aging_cycle is only relevant during evacuation cycle for individual objects and during final mark for // entire regions. Both of these relevant operations occur before final update refs. diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp index af0f55885b1b2..99cea610cc22e 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp @@ -43,20 +43,6 @@ #include "runtime/threads.hpp" #include "utilities/events.hpp" -class ShenandoahFlushAllSATB : public ThreadClosure { -private: - SATBMarkQueueSet& _satb_qset; - -public: - explicit ShenandoahFlushAllSATB(SATBMarkQueueSet& satb_qset) : - _satb_qset(satb_qset) {} - - void do_thread(Thread* thread) override { - // Transfer any partial buffer to the qset for completed buffer processing. - _satb_qset.flush_queue(ShenandoahThreadLocalData::satb_mark_queue(thread)); - } -}; - class ShenandoahPurgeSATBTask : public WorkerTask { public: explicit ShenandoahPurgeSATBTask() : WorkerTask("Purge SATB") { From 584b5323fa5aaf136ff5d783090b3061c35ace17 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 23 Oct 2025 13:50:00 -0700 Subject: [PATCH 12/19] Oops, move inline definition out of ifdef ASSERT --- .../share/gc/shenandoah/shenandoahClosures.inline.hpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp index 36aa55e8c957d..d0035146114f5 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp @@ -253,6 +253,10 @@ inline void ShenandoahConcUpdateRefsClosure::work(T* p) { _heap->conc_update_with_forwarded(p); } +inline void ShenandoahFlushAllSATB::do_thread(Thread* thread) { + // Transfer any partial buffer to the qset for completed buffer processing. + _satb_qset.flush_queue(ShenandoahThreadLocalData::satb_mark_queue(thread)); +} // // ========= Utilities @@ -268,11 +272,6 @@ void ShenandoahAssertNotForwardedClosure::do_oop_work(T* p) { } } -inline void ShenandoahFlushAllSATB::do_thread(Thread* thread) { - // Transfer any partial buffer to the qset for completed buffer processing. - _satb_qset.flush_queue(ShenandoahThreadLocalData::satb_mark_queue(thread)); -} - void ShenandoahAssertNotForwardedClosure::do_oop(narrowOop* p) { do_oop_work(p); } void ShenandoahAssertNotForwardedClosure::do_oop(oop* p) { do_oop_work(p); } #endif From bafd55d1a8337ddcf1ca5e00ba9534126c011aa6 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 23 Oct 2025 14:40:43 -0700 Subject: [PATCH 13/19] Fix assertion --- src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp | 2 ++ src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index b97a4612d3cfa..978a438a079cf 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -1136,6 +1136,8 @@ class ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers final : public Handshake ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers() : HandshakeClosure("Shenandoah Update Thread Roots and Flush SATB"), _flush_all_satb(ShenandoahBarrierSet::satb_mark_queue_set()) { + assert(ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), + "Should be filtering pointers outside of old during old marking"); } void do_thread(Thread* thread) override { diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp index 99cea610cc22e..d3c620fdac6b7 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp @@ -359,8 +359,6 @@ bool ShenandoahOldGeneration::coalesce_and_fill() { void ShenandoahOldGeneration::transfer_pointers_from_satb() const { const ShenandoahHeap* heap = ShenandoahHeap::heap(); assert(heap->is_concurrent_old_mark_in_progress(), "Only necessary during old marking."); - assert(ShenandoahBarrierSet::satb_mark_queue_set().get_filter_out_young(), - "Should be filtering pointers outside of old during old marking"); log_debug(gc)("Transfer SATB buffers"); ShenandoahPurgeSATBTask purge_satb_task; heap->workers()->run_task(&purge_satb_task); From f47e976b1cecd150b31688cb15da39badd262480 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Fri, 24 Oct 2025 11:50:10 -0700 Subject: [PATCH 14/19] Cleanup and comments --- .../gc/shenandoah/shenandoahConcurrentGC.cpp | 43 ++++++++----------- .../share/gc/shenandoah/shenandoahHeap.cpp | 1 + .../gc/shenandoah/shenandoahOldGeneration.hpp | 12 +++--- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index 99b461e6edc23..3bd001726e083 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -669,16 +669,10 @@ void ShenandoahConcurrentGC::op_init_mark() { assert(!heap->has_forwarded_objects(), "No forwarded objects on this path"); if (heap->mode()->is_generational()) { - if (_generation->is_global()) { heap->old_generation()->cancel_gc(); - } else if (heap->is_concurrent_old_mark_in_progress()) { - // Purge the SATB buffers, transferring any valid, old pointers to the - // old generation mark queue. Any pointers not in an old region will be - // abandoned. - // ShenandoahGCPhase phase(ShenandoahPhaseTimings::init_transfer_satb); - // heap->old_generation()->transfer_pointers_from_satb(); } + { // After we swap card table below, the write-table is all clean, and the read table holds // cards dirty prior to the start of GC. Young and bootstrap collection will update @@ -1132,6 +1126,22 @@ void ShenandoahUpdateThreadHandshakeClosure::do_thread(Thread* thread) { } class ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers final : public HandshakeClosure { + // When Shenandoah is marking the old generation, it is possible for the SATB barrier + // to pick up overwritten pointers that point into a cset region. If these pointers + // are accessed by mark threads, they will crash. Once update refs has completed, it is + // no longer possible for a mutator thread to overwrite a pointer into a cset region. + // + // Therefore, at the end of update refs, we use this closure to update the thread roots + // and 'complete' all the thread local SATB buffers. Completing these will filter out + // anything that has already been marked or anything that points to a region which is + // not old. We do not need to worry about ABA situations where a region may become old + // after the pointer is enqueued but before it is filtered. There are only two ways a + // region may become old: + // 1. The region is promoted in place. This is safe because such regions will never + // be in the collection set. If this happens, the pointer will be preserved, essentially + // becoming part of the old snapshot. + // 2. The region is allocated during evacuation of old. This is also not a concern because + // we haven't yet finished marking old so no mixed evacuations will happen. ShenandoahUpdateThreadHandshakeClosure _update_roots; ShenandoahFlushAllSATB _flush_all_satb; @@ -1186,25 +1196,6 @@ void ShenandoahConcurrentGC::op_final_update_refs() { heap->set_has_forwarded_objects(false); if (heap->mode()->is_generational() && heap->is_concurrent_old_mark_in_progress()) { - // When the SATB barrier is left on to support concurrent old gen mark, it may pick up writes to - // objects in the collection set. After those objects are evacuated, the pointers in the - // SATB are no longer safe. Once we have finished update references, we are guaranteed that - // no more writes to the collection set are possible. - // - // This will transfer any old pointers in _active_ regions from the SATB to the old gen - // mark queues. All other pointers will be discarded. This would also discard any pointers - // in old regions that were included in a mixed evacuation. We aren't using the SATB filter - // methods here because we cannot control when they execute. If the SATB filter runs _after_ - // a region has been recycled, we will not be able to detect the bad pointer. - // - // We are not concerned about skipping this step in abbreviated cycles because regions - // with no live objects cannot have been written to and so cannot have entries in the SATB - // buffers. - // - // After update refs, it is no longer possible for a mutator to overwrite a pointer into the collection set - // ShenandoahGCPhase phase(ShenandoahPhaseTimings::final_update_refs_transfer_satb); - // heap->old_generation()->transfer_pointers_from_satb(); - // Aging_cycle is only relevant during evacuation cycle for individual objects and during final mark for // entire regions. Both of these relevant operations occur before final update refs. ShenandoahGenerationalHeap::heap()->set_aging_cycle(false); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index 4566c3ae272be..d855a815d0361 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -2080,6 +2080,7 @@ void ShenandoahHeap::prepare_update_heap_references() { void ShenandoahHeap::propagate_gc_state_to_all_threads() { assert(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "Must be at Shenandoah safepoint"); if (_gc_state_changed) { + // If we are only marking old, we do not need to process young pointers ShenandoahBarrierSet::satb_mark_queue_set().set_filter_out_young( is_concurrent_old_mark_in_progress() && !is_concurrent_young_mark_in_progress() ); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp index 1199b31a7bea2..47dbab416b58c 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp @@ -221,17 +221,19 @@ class ShenandoahOldGeneration : public ShenandoahGeneration { // also cases where the referent of a weak reference ends up in the SATB // and is later collected. In these cases the oop in the SATB buffer becomes // invalid and the _next_ cycle will crash during its marking phase. To - // avoid this problem, we "purge" the SATB buffers during the final update - // references phase if (and only if) an old generation mark is in progress. + // avoid this problem, we "purge" the SATB buffers during a handhsake just + // before the final update references safepoint references phase if (and only if) + // an old generation mark is in progress. + // // At this stage we can safely determine if any of the oops in the SATB // buffer belong to trashed regions (before they are recycled). As it // happens, flushing a SATB queue also filters out oops which have already // been marked - which is the case for anything that is being evacuated // from the collection set. // - // Alternatively, we could inspect the state of the heap and the age of the - // object at the barrier, but we reject this approach because it is likely - // the performance impact would be too severe. + // This method is here for degenerated cycles. A concurrent cycle may be + // cancelled before we have a chance to execute the handshake to flush the + // SATB. void transfer_pointers_from_satb() const; // True if there are old regions waiting to be selected for a mixed collection From 72d89d461dbba350c1e0a60538ac614646389c13 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Fri, 24 Oct 2025 13:16:56 -0700 Subject: [PATCH 15/19] Only flush satb once during degenerated cycle --- .../share/gc/shenandoah/shenandoahDegeneratedGC.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp index 0e42b8628f130..c2044bf2c21a9 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp @@ -160,15 +160,6 @@ void ShenandoahDegenGC::op_degenerated() { _generation->cancel_marking(); } - if (heap->is_concurrent_mark_in_progress()) { - // If either old or young marking is in progress, the SATB barrier will be enabled. - // The SATB buffer may hold a mix of old and young pointers. The old pointers need to be - // transferred to the old generation mark queues and the young pointers are NOT part - // of this snapshot, so they must be dropped here. It is safe to drop them here because - // we will rescan the roots on this safepoint. - heap->old_generation()->transfer_pointers_from_satb(); - } - if (_degen_point == ShenandoahDegenPoint::_degenerated_roots) { // We only need this if the concurrent cycle has already swapped the card tables. // Marking will use the 'read' table, but interesting pointers may have been From 48d4941d5f61942bc6f2b948a7479f3627c82a51 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Fri, 24 Oct 2025 13:24:10 -0700 Subject: [PATCH 16/19] Remove duplicate satb flush closure --- .../share/gc/shenandoah/shenandoahClosures.hpp | 4 ++-- .../gc/shenandoah/shenandoahClosures.inline.hpp | 2 +- .../gc/shenandoah/shenandoahConcurrentGC.cpp | 2 +- .../gc/shenandoah/shenandoahConcurrentMark.cpp | 16 +--------------- .../gc/shenandoah/shenandoahOldGeneration.cpp | 2 +- 5 files changed, 6 insertions(+), 20 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp b/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp index 92a89fc1be991..fefed0340c485 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp @@ -229,12 +229,12 @@ class ShenandoahConcUpdateRefsClosure : public ShenandoahUpdateRefsSuperClosure }; -class ShenandoahFlushAllSATB : public ThreadClosure { +class ShenandoahFlushSATB : public ThreadClosure { private: SATBMarkQueueSet& _satb_qset; public: - explicit ShenandoahFlushAllSATB(SATBMarkQueueSet& satb_qset) : _satb_qset(satb_qset) {} + explicit ShenandoahFlushSATB(SATBMarkQueueSet& satb_qset) : _satb_qset(satb_qset) {} inline void do_thread(Thread* thread) override; }; diff --git a/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp b/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp index d0035146114f5..e8d25b1e5a976 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp @@ -253,7 +253,7 @@ inline void ShenandoahConcUpdateRefsClosure::work(T* p) { _heap->conc_update_with_forwarded(p); } -inline void ShenandoahFlushAllSATB::do_thread(Thread* thread) { +inline void ShenandoahFlushSATB::do_thread(Thread* thread) { // Transfer any partial buffer to the qset for completed buffer processing. _satb_qset.flush_queue(ShenandoahThreadLocalData::satb_mark_queue(thread)); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp index 3bd001726e083..cee8727a3f4de 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp @@ -1143,7 +1143,7 @@ class ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers final : public Handshake // 2. The region is allocated during evacuation of old. This is also not a concern because // we haven't yet finished marking old so no mixed evacuations will happen. ShenandoahUpdateThreadHandshakeClosure _update_roots; - ShenandoahFlushAllSATB _flush_all_satb; + ShenandoahFlushSATB _flush_all_satb; public: ShenandoahUpdateThreadRootsAndFlushOldSatbBuffers() : diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp index 7a195f64cbd51..367a15abfa49a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp @@ -66,20 +66,6 @@ class ShenandoahConcurrentMarkingTask : public WorkerTask { } }; -class ShenandoahSATBAndRemarkThreadsClosure : public ThreadClosure { -private: - SATBMarkQueueSet& _satb_qset; - -public: - explicit ShenandoahSATBAndRemarkThreadsClosure(SATBMarkQueueSet& satb_qset) : - _satb_qset(satb_qset) {} - - void do_thread(Thread* thread) override { - // Transfer any partial buffer to the qset for completed buffer processing. - _satb_qset.flush_queue(ShenandoahThreadLocalData::satb_mark_queue(thread)); - } -}; - template class ShenandoahFinalMarkingTask : public WorkerTask { private: @@ -109,7 +95,7 @@ class ShenandoahFinalMarkingTask : public WorkerTask { while (satb_mq_set.apply_closure_to_completed_buffer(&cl)) {} assert(!heap->has_forwarded_objects(), "Not expected"); - ShenandoahSATBAndRemarkThreadsClosure tc(satb_mq_set); + ShenandoahFlushSATB tc(satb_mq_set); Threads::possibly_parallel_threads_do(true /* is_par */, &tc); } _cm->mark_loop(worker_id, _terminator, GENERATION, false /*not cancellable*/, diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp index d3c620fdac6b7..a1e17f4f3812a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp @@ -52,7 +52,7 @@ class ShenandoahPurgeSATBTask : public WorkerTask { void work(uint worker_id) override { ShenandoahParallelWorkerSession worker_session(worker_id); ShenandoahSATBMarkQueueSet &satb_queues = ShenandoahBarrierSet::satb_mark_queue_set(); - ShenandoahFlushAllSATB flusher(satb_queues); + ShenandoahFlushSATB flusher(satb_queues); Threads::possibly_parallel_threads_do(true /* is_par */, &flusher); } }; From ec58b72f1fa4a7f609eb13f8f84e7a39e7e01838 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Fri, 24 Oct 2025 13:52:23 -0700 Subject: [PATCH 17/19] Fix typo in comment --- src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp index 47dbab416b58c..c8064c6aa779b 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp @@ -221,7 +221,7 @@ class ShenandoahOldGeneration : public ShenandoahGeneration { // also cases where the referent of a weak reference ends up in the SATB // and is later collected. In these cases the oop in the SATB buffer becomes // invalid and the _next_ cycle will crash during its marking phase. To - // avoid this problem, we "purge" the SATB buffers during a handhsake just + // avoid this problem, we "purge" the SATB buffers during a handshake just // before the final update references safepoint references phase if (and only if) // an old generation mark is in progress. // From ef21a81c46cba1fd40bf04a4c76a6f8b914a90e5 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Tue, 28 Oct 2025 13:29:14 -0700 Subject: [PATCH 18/19] Flush SATB buffers upon entering degenerated cycle when old marking is in progress This has to happen at least once during the degenerated cycle. Doing it at the start, rather than the end, simplifies the verifier. --- .../share/gc/shenandoah/shenandoahDegeneratedGC.cpp | 9 ++++++--- .../share/gc/shenandoah/shenandoahGenerationalHeap.cpp | 6 ------ 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp index c2044bf2c21a9..6bed188535c4d 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp @@ -95,12 +95,16 @@ void ShenandoahDegenGC::op_degenerated() { // some phase, we have to upgrade the Degenerate GC to Full GC. heap->clear_cancelled_gc(); -#ifdef ASSERT if (heap->mode()->is_generational()) { - ShenandoahOldGeneration* old_generation = heap->old_generation(); + const ShenandoahOldGeneration* old_generation = heap->old_generation(); if (!heap->is_concurrent_old_mark_in_progress()) { // If we are not marking the old generation, there should be nothing in the old mark queues assert(old_generation->task_queues()->is_empty(), "Old gen task queues should be empty"); + } else { + // This is still necessary for degenerated cycles because the degeneration point may occur + // after final mark of the young generation. See ShenandoahConcurrentGC::op_final_update_refs for + // a more detailed explanation. + old_generation->transfer_pointers_from_satb(); } if (_generation->is_global()) { @@ -112,7 +116,6 @@ void ShenandoahDegenGC::op_degenerated() { "Old generation cannot be in state: %s", old_generation->state_name()); } } -#endif ShenandoahMetricsSnapshot metrics(heap->free_set()); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp index bc653b030a8ca..4f8305b6d0025 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp @@ -1053,12 +1053,6 @@ void ShenandoahGenerationalHeap::final_update_refs_update_region_states() { void ShenandoahGenerationalHeap::complete_degenerated_cycle() { shenandoah_assert_heaplocked_or_safepoint(); - if (is_concurrent_old_mark_in_progress()) { - // This is still necessary for degenerated cycles because the degeneration point may occur - // after final mark of the young generation. See ShenandoahConcurrentGC::op_final_update_refs for - // a more detailed explanation. - old_generation()->transfer_pointers_from_satb(); - } // We defer generation resizing actions until after cset regions have been recycled. TransferResult result = balance_generations(); From c741bf6b0c109806e7631583cc29e9edf96428d9 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Thu, 6 Nov 2025 10:30:19 -0800 Subject: [PATCH 19/19] Improve comment describing the need for a method to filter SATB buffers for degenerated cycles --- .../gc/shenandoah/shenandoahOldGeneration.hpp | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp index eb5dd250b5b12..cd78ed4ef5e2e 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp @@ -228,26 +228,26 @@ class ShenandoahOldGeneration : public ShenandoahGeneration { // Cancels old gc and transitions to the idle state void cancel_gc(); - // We leave the SATB barrier on for the entirety of the old generation - // marking phase. In some cases, this can cause a write to a perfectly - // reachable oop to enqueue a pointer that later becomes garbage (because - // it points at an object that is later chosen for the collection set). There are - // also cases where the referent of a weak reference ends up in the SATB - // and is later collected. In these cases the oop in the SATB buffer becomes - // invalid and the _next_ cycle will crash during its marking phase. To - // avoid this problem, we "purge" the SATB buffers during a handshake just - // before the final update references safepoint references phase if (and only if) - // an old generation mark is in progress. + // The SATB barrier will be "enabled" until old marking completes. This means it is + // possible for an entire young collection cycle to execute while the SATB barrier is enabled. + // Consider a situation like this, where we have a pointer 'B' at an object 'A' which is in + // the young collection set: // - // At this stage we can safely determine if any of the oops in the SATB - // buffer belong to trashed regions (before they are recycled). As it - // happens, flushing a SATB queue also filters out oops which have already - // been marked - which is the case for anything that is being evacuated - // from the collection set. + // +--Young, CSet------+ +--Young, Regular----+ + // | | | | + // | | | | + // | A <--------------------+ B | + // | | | | + // | | | | + // +-------------------+ +--------------------+ // - // This method is here for degenerated cycles. A concurrent cycle may be - // cancelled before we have a chance to execute the handshake to flush the - // SATB. + // If a mutator thread overwrites pointer B, the SATB barrier will dutifully enqueue + // object A. However, this object will be trashed when the young cycle completes. We must, + // therefore, filter this object from the SATB buffer before any old mark threads see it. + // We do this with a handshake before final-update-refs (see shenandoahConcurrentGC.cpp). + // + // This method is here only for degenerated cycles. A concurrent cycle may be cancelled before + // we have a chance to execute the handshake to flush the SATB in final-update-refs. void transfer_pointers_from_satb() const; // True if there are old regions waiting to be selected for a mixed collection