From 295281c7262fb8a4cd1db88f4781f29e2fa15747 Mon Sep 17 00:00:00 2001 From: Xiaolong Peng Date: Wed, 3 Jun 2026 15:13:12 -0700 Subject: [PATCH] fix: Reserve promotion budget before allocation, refund on failure --- .../share/gc/shenandoah/shenandoahHeap.cpp | 19 +++++++++++-------- .../gc/shenandoah/shenandoahOldGeneration.cpp | 4 ---- .../gc/shenandoah/shenandoahOldGeneration.hpp | 6 ------ .../shenandoahPartitionAllocator.cpp | 2 +- .../test_shenandoahOldGeneration.cpp | 12 ------------ 5 files changed, 12 insertions(+), 31 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp index 74514044bab..e7108c07146 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp @@ -1027,6 +1027,13 @@ inline bool ShenandoahHeap::should_retry_allocation(size_t original_full_gc_coun } HeapWord* ShenandoahHeap::allocate_memory_work(ShenandoahAllocRequest& req, bool& in_new_region) { + // Reserve the promotion budget up front so it is enforced atomically without the heap lock. + // If the reserve is exhausted, deny the promotion rather than overshoot it; the reservation + // is refunded below if the allocation itself fails. + if (req.is_promotion() && !old_generation()->try_expend_promoted(req.size() << LogHeapWordSize)) { + return nullptr; + } + HeapWord* result = _allocator->allocate(req, in_new_region); if (result != nullptr) { @@ -1038,17 +1045,13 @@ HeapWord* ShenandoahHeap::allocate_memory_work(ShenandoahAllocRequest& req, bool if (req.is_lab_alloc()) { old_generation()->configure_plab_for_current_thread(req); } else if (req.is_promotion()) { - const size_t actual_size = req.actual_size() * HeapWordSize; - // The object has already been promoted into old-gen, so account for it unconditionally. - // can_allocate() gated this promotion against the reserve under the heap lock, but the - // lock is released before we get here; a conditional reservation could fail under - // concurrent expends and silently lose accounting, leading to unbounded over-promotion. - log_debug(gc, plab)("Expend shared promotion of %zu bytes", actual_size); - old_generation()->expend_promoted(actual_size); + log_debug(gc, plab)("Expend shared promotion of %zu bytes", req.actual_size() * HeapWordSize); } } + } else if (req.is_promotion()) { + // Allocation failed, so refund the promotion budget reserved above. + old_generation()->unexpend_promoted(req.size() << LogHeapWordSize); } - return result; } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp index 33768f7dded..52121e4b2ed 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp @@ -206,10 +206,6 @@ bool ShenandoahOldGeneration::try_expend_promoted(size_t increment) { return false; } -size_t ShenandoahOldGeneration::expend_promoted(size_t increment) { - return _promoted_expended.add_then_fetch(increment); -} - size_t ShenandoahOldGeneration::unexpend_promoted(size_t decrement) { return _promoted_expended.sub_then_fetch(decrement); } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp index 9b1fa350d64..f7f0a0ae0ba 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp @@ -116,12 +116,6 @@ public: // Use this to gate a promotion decision before promoting. bool try_expend_promoted(size_t increment); - // Unconditionally account `increment` bytes that have already been promoted into old-gen. - // Unlike try_expend_promoted, this never fails: the promotion has happened, so the budget - // must reflect it to stay truthful (it may briefly overshoot the reserve under concurrency). - // Lock-free: safe to call without the heap lock. - size_t expend_promoted(size_t increment); - // This is used to return unused memory from a retired promotion LAB size_t unexpend_promoted(size_t decrement); diff --git a/src/hotspot/share/gc/shenandoah/shenandoahPartitionAllocator.cpp b/src/hotspot/share/gc/shenandoah/shenandoahPartitionAllocator.cpp index 464438f1e7e..cfe958fef90 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahPartitionAllocator.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahPartitionAllocator.cpp @@ -44,7 +44,7 @@ HeapWord* ShenandoahPartitionAllocator::allocate(ShenandoahAllocReque // OldCollector: verify old generation has room before attempting allocation. if constexpr (PARTITION == ShenandoahFreeSetPartitionId::OldCollector) { - if (!ShenandoahHeap::heap()->old_generation()->can_allocate(req)) { + if (!req.is_promotion() && !ShenandoahHeap::heap()->old_generation()->can_allocate(req)) { return nullptr; } } diff --git a/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldGeneration.cpp b/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldGeneration.cpp index 853b669de5e..483464fba17 100644 --- a/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldGeneration.cpp +++ b/test/hotspot/gtest/gc/shenandoah/test_shenandoahOldGeneration.cpp @@ -179,16 +179,4 @@ TEST_VM_F(ShenandoahOldGenerationTest, test_try_expend_promoted_should_increase_ EXPECT_EQ(expended_before + 128, expended_after) << "Should expend promotion"; } -TEST_VM_F(ShenandoahOldGenerationTest, test_expend_promoted_accounts_unconditionally) { - SKIP_IF_NOT_SHENANDOAH(); - // try_expend_promoted gates a promotion decision and must fail past the reserve... - const size_t over = old->get_promoted_reserve() + 1; - size_t expended_before = old->get_promoted_expended(); - EXPECT_FALSE(old->try_expend_promoted(over)) << "Reservation must fail past the reserve"; - EXPECT_EQ(expended_before, old->get_promoted_expended()) << "Failed reservation must not change accounting"; - // ...but expend_promoted accounts bytes already promoted unconditionally, even past the reserve. - EXPECT_EQ(expended_before + over, old->expend_promoted(over)) - << "Already-promoted bytes must be accounted even past the reserve"; -} - #undef SKIP_IF_NOT_SHENANDOAH