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

[Albert Mingkun Yang]

On Sun, 18 May 2025 17:21:40 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> so that we can do the check after all the GCs

Well, not really. In the old impl, `GCOverheadChecker::check_gc_overhead_limit` calls `set_gc_overhead_limit_exceeded` only for full-gc.

 > 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.

I believe so.  In the old impl, we don't check gc-overhead for explicit gcs. Only allocation-failure caused gcs are interesting, which all go through `satisfy_failed_allocation`.


  // Ignore explicit GC's.  Exiting here does not set the flag and
  // does not reset the count.
  if (GCCause::is_user_requested_gc(gc_cause) ||
      GCCause::is_serviceability_requested_gc(gc_cause)) {
    return;
  }

> 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?

I don't see an easy to re-structure the code while keeping all the relevant logic in the current context. I added some comments; check if it makes the flow easier to follow.

> 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");` ?

Revised; needs to be `>=` though.

> 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.

Added checking for `from_space`.

If all live-objs don't fit in old-gen, leftovers will be kept in its own space.


  // Summarize the remaining spaces in the young gen.  The initial target space
  // is the old gen.  If a space does not fit entirely into the target, then the
  // remainder is compacted into the space itself and that space becomes the new
  // target.

> 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?

Yes; when testing/developing, I don't find them to be very useful.

> 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?

It's more to convey the intend that this number have a lower bound if future changes want to lower it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094893944
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094867831
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094870461
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094882156
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094882750
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094883781