From 09b25cd0a24a4eaddce49917d958adc667ab5465 Mon Sep 17 00:00:00 2001 From: Albert Mingkun Yang Date: Mon, 17 Nov 2025 09:38:17 +0000 Subject: [PATCH] 8371465: Parallel: Revise asserts around heap expansion Reviewed-by: aboldtch, tschatzl --- .../share/gc/parallel/mutableSpace.cpp | 13 -------- .../share/gc/parallel/mutableSpace.hpp | 5 --- .../gc/parallel/parallelScavengeHeap.cpp | 9 +++++ src/hotspot/share/gc/parallel/psOldGen.cpp | 33 +++++++++++++++---- 4 files changed, 36 insertions(+), 24 deletions(-) diff --git a/src/hotspot/share/gc/parallel/mutableSpace.cpp b/src/hotspot/share/gc/parallel/mutableSpace.cpp index 6d30c5a8d1f..fc42fc1eab2 100644 --- a/src/hotspot/share/gc/parallel/mutableSpace.cpp +++ b/src/hotspot/share/gc/parallel/mutableSpace.cpp @@ -180,19 +180,6 @@ bool MutableSpace::cas_deallocate(HeapWord *obj, size_t size) { return AtomicAccess::cmpxchg(top_addr(), expected_top, obj) == expected_top; } -// Only used by oldgen allocation. -bool MutableSpace::needs_expand(size_t word_size) const { - // This method can be invoked either outside of safepoint by java threads or - // in safepoint by gc workers. Such accesses are synchronized by holding one - // of the following locks. - assert(Heap_lock->is_locked() || PSOldGenExpand_lock->is_locked(), "precondition"); - - // Holding the lock means end is stable. So while top may be advancing - // via concurrent allocations, there is no need to order the reads of top - // and end here, unlike in cas_allocate. - return pointer_delta(end(), top()) < word_size; -} - void MutableSpace::oop_iterate(OopIterateClosure* cl) { HeapWord* obj_addr = bottom(); HeapWord* t = top(); diff --git a/src/hotspot/share/gc/parallel/mutableSpace.hpp b/src/hotspot/share/gc/parallel/mutableSpace.hpp index 37fa7e3710e..9d3894e2489 100644 --- a/src/hotspot/share/gc/parallel/mutableSpace.hpp +++ b/src/hotspot/share/gc/parallel/mutableSpace.hpp @@ -127,11 +127,6 @@ public: virtual HeapWord* cas_allocate(size_t word_size); // Optional deallocation. Used in NUMA-allocator. bool cas_deallocate(HeapWord *obj, size_t size); - // Return true if this space needs to be expanded in order to satisfy an - // allocation request of the indicated size. Concurrent allocations and - // resizes may change the result of a later call. Used by oldgen allocator. - // precondition: holding PSOldGenExpand_lock if not VM thread - bool needs_expand(size_t word_size) const; // Iteration. void oop_iterate(OopIterateClosure* cl); diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp index f5ed40c40e5..ebaea3ecba4 100644 --- a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp +++ b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp @@ -403,7 +403,16 @@ bool ParallelScavengeHeap::check_gc_overhead_limit() { } HeapWord* ParallelScavengeHeap::expand_heap_and_allocate(size_t size, bool is_tlab) { +#ifdef ASSERT assert(Heap_lock->is_locked(), "precondition"); + if (is_init_completed()) { + assert(SafepointSynchronize::is_at_safepoint(), "precondition"); + assert(Thread::current()->is_VM_thread(), "precondition"); + } else { + assert(Thread::current()->is_Java_thread(), "precondition"); + assert(Heap_lock->owned_by_self(), "precondition"); + } +#endif HeapWord* result = young_gen()->expand_and_allocate(size); diff --git a/src/hotspot/share/gc/parallel/psOldGen.cpp b/src/hotspot/share/gc/parallel/psOldGen.cpp index 4e614c53447..974cd6aca59 100644 --- a/src/hotspot/share/gc/parallel/psOldGen.cpp +++ b/src/hotspot/share/gc/parallel/psOldGen.cpp @@ -33,6 +33,7 @@ #include "gc/shared/spaceDecorator.hpp" #include "logging/log.hpp" #include "oops/oop.inline.hpp" +#include "runtime/init.hpp" #include "runtime/java.hpp" #include "utilities/align.hpp" @@ -118,13 +119,22 @@ void PSOldGen::initialize_performance_counters() { } HeapWord* PSOldGen::expand_and_allocate(size_t word_size) { +#ifdef ASSERT assert(Heap_lock->is_locked(), "precondition"); + if (is_init_completed()) { + assert(SafepointSynchronize::is_at_safepoint(), "precondition"); + assert(Thread::current()->is_VM_thread(), "precondition"); + } else { + assert(Thread::current()->is_Java_thread(), "precondition"); + assert(Heap_lock->owned_by_self(), "precondition"); + } +#endif - if (object_space()->needs_expand(word_size)) { + if (pointer_delta(object_space()->end(), object_space()->top()) < word_size) { expand(word_size*HeapWordSize); } - // Reuse the CAS API even though this is VM thread in safepoint. This method + // Reuse the CAS API even though this is in a critical section. This method // is not invoked repeatedly, so the CAS overhead should be negligible. return cas_allocate_noexpand(word_size); } @@ -168,7 +178,7 @@ bool PSOldGen::expand_for_allocate(size_t word_size) { // true until we expand, since we have the lock. Other threads may take // the space we need before we can allocate it, regardless of whether we // expand. That's okay, we'll just try expanding again. - if (object_space()->needs_expand(word_size)) { + if (pointer_delta(object_space()->end(), object_space()->top()) < word_size) { result = expand(word_size*HeapWordSize); } } @@ -192,10 +202,21 @@ void PSOldGen::try_expand_till_size(size_t target_capacity_bytes) { bool PSOldGen::expand(size_t bytes) { #ifdef ASSERT - if (!Thread::current()->is_VM_thread()) { - assert_lock_strong(PSOldGenExpand_lock); + // During startup (is_init_completed() == false), expansion can occur for + // 1. java-threads invoking heap-allocation (using Heap_lock) + // 2. CDS construction by a single thread (using PSOldGenExpand_lock but not needed) + // + // After startup (is_init_completed() == true), expansion can occur for + // 1. GC workers for promoting to old-gen (using PSOldGenExpand_lock) + // 2. VM thread to satisfy the pending allocation + // Both cases are inside safepoint pause, but are never overlapping. + // + if (is_init_completed()) { + assert(SafepointSynchronize::is_at_safepoint(), "precondition"); + assert(Thread::current()->is_VM_thread() || PSOldGenExpand_lock->owned_by_self(), "precondition"); + } else { + assert(Heap_lock->owned_by_self() || PSOldGenExpand_lock->owned_by_self(), "precondition"); } - assert_locked_or_safepoint(Heap_lock); assert(bytes > 0, "precondition"); #endif const size_t remaining_bytes = virtual_space()->uncommitted_size();