RFR: 8238687: Investigate memory uncommit during young collections in G1 [v5]

[Thomas Schatzl]

On Thu, 26 Jun 2025 13:28:13 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Hi all,
>> 
>> Please review this change to the G1 heap resizing policy, aimed at improving alignment with the configured GCTimeRatio. The GCTimeRatio is intended to manage the balance between GC time and Application execution time. G1's current implementation of GCTimeRatio appears to have drifted from its intended purpose over time.  Therefore, we need to change G1’s use of the GCTimeRatio to better manage heap sizes without relying on additional magic constants.
>> 
>> The primary goal is to enable both heap expansion and shrinking at the end of any GC, rather than limiting shrinking to only the Remark or Full GC pauses as is currently done. We achieve this using heuristics that monitor both short-term and long-term GC time ratios relative to the configured GCTimeRatio.
>> 
>> - The short-term policy adjusts a counter based on whether recent GC time is above or below a target range around GCTimeRatio (as defined by G1MinimumPercentOfGCTimeRatio). When the counter crosses predefined thresholds, the heap may be expanded or shrunk accordingly.
>> 
>> - The long-term policy evaluates the GC time ratio over a long-term interval and triggers resizing if the number of recorded ratios exceeds a threshold and the GC time ratio over the long-term interval is outside the target range.
>> 
>> - These heuristics allow for responsive heap resizing (both expansion and shrinking) at the end of any GC, guided by actual GC performance rather than fixed thresholds or constants.
>> 
>> We are increasing the default GCTimeRatio from 12 to 24, since under the new policy, the current default leads to overly aggressive heap shrinking as the GCTimeRatio allows for a lot more GC overhead.
>> 
>> Additionally, we are removing the heap resizing step at the end of the Remark pause which was based on MinHeapFreeRatio and MaxHeapFreeRatio. We keep this MinHeapFreeRatio-MaxHeapFreeRatio based resizing logic at the end of Full GC and Remark pauses that may have been triggered by PeriodicGCs.
>> 
>> As a result of these changes, some applications may settle at more appropriate and in some cases smaller heap sizes for the configured GCTimeRatio. While this may appear as a regression in some benchmarks that are sensitive to heap size, it represents more accurate G1 behavior with respect to the GCTimeRatio. Although smaller heap sizes may lead to more frequent GCs, this is the expected outcome, provided the cumulative GC overhead remains within the limits defined by the GCTimeRatio.
>> 
>> Testing: Mach5 ...
>
> Ivan Walulya has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reviews

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 104:

> 102: }
> 103: 
> 104: // Computes a smooth scaling factor based on the relative deviation of observed gc_cpu_usage

Typically the code uses "actual" instead of "observed". There are also a few "current" `gc_cpu_usage`thrown in. If possible, it would be nice to harmonize usage in the documentation.

src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 129:

> 127: //
> 128: // This helps avoid overreacting to small gc_cpu_usage deviations but respond appropriately
> 129: // when necessary.

This sentence seems to be a repeat of the one above ("This ensures appropriate heap resizing when deviations become significant, while avoiding overreacting to minor deviations.")

I would remove the first occurrence (maybe keeping the first version).

src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 151:

> 149:   size_t uncommitted_bytes = reserved_bytes - committed_bytes;
> 150:   size_t expand_bytes_via_pct =
> 151:     uncommitted_bytes * G1ExpandByPercentOfAvailable / 100;

I think this linebreak is unnecessary, feel free to keep though.

src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 190:

> 188:   uint target_regions_to_shrink = _g1h->num_free_regions();
> 189: 
> 190:   uint reserve_regions = ceil(_g1h->num_committed_regions() * G1ReservePercent / 100.0);

This is unused except for the log message. I think we once discussed this value, and removed its use because we did not know its reason. It should be removed completely, even from the log message.

src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 268:

> 266: 
> 267:   log_debug(gc, ergo, heap)("Heap triggers: pauses-since-start: %u num-prev-pauses-for-heuristics: %u GC CPU usage deviation counter: %d",
> 268:                             _recent_cpu_usage_deltas.num(), long_term_count_limit(), _gc_cpu_usage_deviation_counter);

`pauses-since-start` is a misnomer, it's how many deltas were collectors; the second is a maximum (maybe print that once as precious log among other relevant information for this kind of ergonomics?).

src/hotspot/share/gc/g1/g1HeapSizingPolicy.hpp line 49:

> 47: // If below that range, we decrement that counter, if above, we increment it.
> 48: // The intent of this mechanism is to filter short term events because heap sizing has
> 49: // some overhead.

I think that sentence should move just before the full collection handling description.

src/hotspot/share/gc/g1/g1HeapSizingPolicy.hpp line 54:

> 52: // if that counter reaches -G1CPUUsageShrinkThreshold we consider shrinking the heap.
> 53: //
> 54: // While doing so, we accumulate the relative difference to the gc_cpu_usage_target

`gc_cpu_usage_target` has not been defined before, although above mentions "the target GC CPU usage". It seems better to just use the words here too.

src/hotspot/share/gc/g1/g1HeapSizingPolicy.hpp line 64:

> 62: // Long term behavior is solely managed by regularly comparing actual long term
> 63: // GC CPU usage with the boundaries of above range in regular long term intervals.
> 64: // If current long term GC CPU usage is outside, expand or shrink respectively.

Suggestion:

// If current long term GC CPU usage is different to the target, expand or shrink respectively.

src/hotspot/share/gc/g1/jvmFlagConstraintsG1.cpp line 215:

> 213: }
> 214: 
> 215: JVMFlag::Error gc_cpu_usage_threshold_healper(JVMFlagsEnum flagid,

Suggestion:

JVMFlag::Error gc_cpu_usage_threshold_helper(JVMFlagsEnum flagid,

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

PR Review: https://git.openjdk.org/jdk/pull/25832#pullrequestreview-2962629782
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172058191
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172065270
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172066367
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172071149
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172080088
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172040409
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172043739
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2172045132
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2169389589