RFR: 8338977: Parallel: Improve heap resizing heuristics [v3]

Sun May 18 18:08:56 UTC 2025

On Sun, 18 May 2025 15:20:50 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> This patch refines Parallel's sizing strategy to improve overall memory management and performance.
>> 
>> The young generation layout has been reconfigured from the previous `eden-from/to` arrangement to a new `from/to-eden` order. This new layout facilitates young generation resizing, since we perform resizing after a successful young GC when all live objects are located at the beginning of the young generation. Previously, resizing was often inhibited by live objects residing in the middle of the young generation (from-space). The new layout is illustrated in `parallelScavengeHeap.hpp`.
>> 
>> `NumberSeq` is now used to track various runtime metrics, such as minor/major GC pause durations, promoted/survived bytes after a young GC, highest old generation usage, etc. This tracking primarily lives in `AdaptiveSizePolicy` and its subclass `PSAdaptiveSizePolicy`.
>> 
>> GC overhead checking, which was previously entangled with adaptive resizing logic, has been extracted and is now largely encapsulated in `ParallelScavengeHeap::is_gc_overhead_limit_reached`.
>> 
>> ## Performance evaluation
>> 
>> - SPECjvm2008-Compress shows ~8% improvement on Linux/AArch64 and Linux/x64 (restoring the regression reported in [JDK-8332485](https://bugs.openjdk.org/browse/JDK-8332485) and [JDK-8338689](https://bugs.openjdk.org/browse/JDK-8338689)).
>> - Fixes the surprising behavior when using a non-default (smaller) value of `GCTimeRatio` with Heapothesys/Hyperalloc, as discussed in [this thread](https://mail.openjdk.org/pipermail/hotspot-gc-dev/2024-November/050146.html).
>> - Performance is mostly neutral across other tested benchmarks: **DaCapo**, **SPECjbb2005**, **SPECjbb2015**, **SPECjvm2008**, and **CacheStress**. The number of young-gc sometimes goes up a bit and the total heap-size decreases a bit, because promotion-size-to-old-gen goes down with the more effective eden/survivor-space resizing.
>> 
>> PS: I have opportunistically set the obsolete/expired version to 25/26 for now. I will update them accordingly before merging.
>> 
>> Test: tier1-8
>
> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into pgc-size-policy
>  - review
>  - Merge branch 'master' into pgc-size-policy
>  - pgc-size-policy

Some more comments.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 343:

> 341:       if (_gc_overhead_counter >= GCOverheadLimitThreshold) {
> 342:         return nullptr;
> 343:       }

Returning `nullptr` means the `OutOfMemoryException` should be thrown later. Is it good to add a `error` level log here?

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 363:

> 361: }
> 362: 
> 363: bool ParallelScavengeHeap::check_gc_overhead_limit() {

In main-line code, the method `check_gc_overhead_limit` is invoked by `PSScavenge::invoke` and `PSParallelCompact::invoke_no_policy` so that we can do the check after all the GCs. But now you only use `check_gc_overhead_limit` in `ParallelScavengeHeap::satisfy_failed_allocation`. I suspect whether it can check the gc overhead limit accurately.

src/hotspot/share/gc/parallel/psYoungGen.cpp line 268:

> 266:   size_t original_committed_size = virtual_space()->committed_size();
> 267: 
> 268:   while (true) {

The `while` statement only runs once. Maybe we can find a better way to handle such complex conditional flow.

src/hotspot/share/gc/parallel/psYoungGen.cpp line 268:

> 266:   size_t original_committed_size = virtual_space()->committed_size();
> 267: 
> 268:   while (true) {

The `while` statement only runs once. May we find a better way to refactor the code?

src/hotspot/share/gc/parallel/psYoungGen.cpp line 334:

> 332:   assert(from_space()->capacity_in_bytes() == to_space()->capacity_in_bytes(), "inv");
> 333:   const size_t current_survivor_size = from_space()->capacity_in_bytes();
> 334:   assert(max_gen_size() > 2 * current_survivor_size, "inv");

Should this assertion be changed to `assert(max_gen_size() > current_eden_size + 2 * current_survivor_size, "inv");` ?

src/hotspot/share/gc/parallel/psYoungGen.cpp line 379:

> 377:   // We usually resize young-gen only after a successful young-gc. However, in emergency state, we wanna expand young-gen to its max-capacity.
> 378:   // Young-gen should be empty normally after a full-gc.
> 379:   if (eden_space()->is_empty() && to_space()->is_empty()) {

Why don't you test the `from space` here? And actually, if the `eden space` is empty, the `from space` and `to space` are empty too, because the objects are firstly moved to `eden space`. See the method `PSParallelCompact::summary_phase` for more information. So here, you only need to test whether the `eden space` is empty.

src/hotspot/share/gc/parallel/psYoungGen.cpp line 487:

> 485: 
> 486: void PSYoungGen::resize_spaces(size_t requested_eden_size,
> 487:                                size_t requested_survivor_size) {

You remove some `trace` level logs in this method. Please confirm whether it is your intent?

src/hotspot/share/gc/shared/adaptiveSizePolicy.hpp line 48:

> 46:   // Default: 100ms.
> 47:   static constexpr double MinGCDistanceSecond = 0.100;
> 48:   static_assert(MinGCDistanceSecond >= 0.001, "inv");

The`MinGCDistanceSecond` is just a contant; the static assertion seems unnecessary?

-------------

Changes requested by gli (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25000#pullrequestreview-2848994453
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094590507
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094582368
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094559809
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094559817
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094560936
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094567980
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094571784
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094574692