RFR: 8371465: Parallel: Revise asserts around heap expansion
Axel Boldt-Christmas
aboldtch at openjdk.org
Thu Nov 13 08:21:14 UTC 2025
On Wed, 12 Nov 2025 13:14:34 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:
> Revise heap/old-gen expansion asserts/comments to cover two paths:
>
> 1. startup: no gc can occur
> 2. after-startup: during safepoint, by vm-thread.
>
> Test: tier1-4
Alright so if I try to understand the invariants correct here surrounding the `_end`.
For using unordered reads and modifying the value:
* Before GC can run
* Mapping AOT archive has no concurrency
* Will take `PSOldGenExpand_lock` unnecessarily, but not the `Heap_lock`
* assert_locked_or_safepoint(Heap_lock) is skipped because `!Universe::is_fully_initialized()`
* Normal failed allocations (may be while loading Streamed AOT archive)
* Holds the `Heap_lock`
* After the GC can run
* In the VM thread of a GC safepoint (no parallel GC threads are running)
* By a GC thread during a GC
* Uses `PSOldGenExpand_lock`
Looking at the code and writing this out I can see that only half of my concerns raised in #28162 / [JDK-8371369](https://bugs.openjdk.org/browse/JDK-8371369) were addressed.
The asserts in `MutableSpace::needs_expand` were addressed, but not the once in `PSOldGen::expand`. Right now if we are in no GC phase (`!is_init_completed()`) and we would fail to expand the young gen, and attempt to expand the old gen we would hit the assert inside `PSOldGen::expand` which says that any thread other than the VM thread must also hold `PSOldGenExpand_lock`. Which we do not in the allocation path.
My guess is that this scenario is extremely unlikely to happen. However I think the assert inside `PSOldGen::expand` should be conditioned on `if (!Thread::current()->is_VM_thread() && !is_init_completed())`.
Other than that I think these new invariants assert looks good. But some of the interactions between `PSOldGenExpand_lock`, `Heap_lock` are not obvious at a glance.
May we should have some comment (or maybe there already is) describing how this works.
Something along the lines of:
Before `is_init_completed()` all expansion is performed while holding the `Heap_lock`.
With the exception of when the AOT heap archive is mapped into the heap.
After `is_init_completed()` all expansions are either preformed by active GC threads holding the `PSOldGenExpand_lock` or the VM thread in a GC safepoint. The GC threads' and the VM thread's expansion will never occur concurrently.
_Note here we might just want to take the `Heap_lock` in `ParallelScavengeHeap::allocate_loaded_archive_space` and keep this invariant, just because it simplifies the invariant and we can remove the mapped AOT archive exception line._
-------------
PR Review: https://git.openjdk.org/jdk/pull/28266#pullrequestreview-3458284737
More information about the hotspot-gc-dev
mailing list