On Sun, 18 May 2025 15:20:50 GMT, Albert Mingkun Yang <ay...@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
