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

[Kim Barrett]

On Thu, 19 Jun 2025 12:26:15 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:
> 
>   Albert suggestions

Changes requested by kbarrett (Reviewer).

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 894:

> 892: 
> 893:   bool should_expand;
> 894:   size_t resize_bytes = _heap_sizing_policy->full_collection_resize_amount(should_expand, allocation_word_size);

pre-existing:  I wonder why this isn't a function that returns a `ptrdiff_t` delta on the current size,
removing the need for multiple values, one being returned via a by-reference out parameter.
Similarly for the young collection case. Or return the size & direction as a pair-like object.
(Personally, I find by-reference out parameters confusing to read, but maybe that's just me.)

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1028:

> 1026: 
> 1027:   uint num_regions_to_expand = (uint)(aligned_expand_bytes / G1HeapRegion::GrainBytes);
> 1028:   assert(num_regions_to_expand > 0, "Must expand by at least one region");

This assert seems like needless clutter to me. We just aligned up, and then we divided.
A (to me) more useful assert would be up-front that `word_size` > 0.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1042:

> 1040:   uint expanded_by = _hrm.expand_by(num_regions_to_expand, pretouch_workers);
> 1041: 
> 1042:   assert(expanded_by > 0, "must have failed during commit.");

pre-existing: Using an assert to detect and "handle" this seems wrong. This seems like something that
simply can happen? And so should be dealt with in some graceful fashion. It actually seems like things
would more or less work if the assert was just removed.

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 563:

> 561:   void resize_heap_after_young_collection(size_t allocation_word_size);
> 562:   void resize_heap_after_full_collection(size_t allocation_word_size);
> 563:   void resize_heap(size_t resize_bytes, bool should_expand);

I think `resize_heap` is a helper for the other two? So shouldn't be public.

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1465:

> 1463: 
> 1464:     if (_g1h->last_gc_was_periodic()) {
> 1465:       _g1h->resize_heap_after_full_collection(size_t(0) /* allocation_word_size */);

I don't think a cast is needed here.

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

> 91:   // Sigmoid Parameters:
> 92:   double inflection_point = 1.0; // Inflection point where acceleration begins (midpoint of sigmoid).
> 93:   double steepness = 6.0;

Curious as to how these constants were determined. Maybe a comment about that?

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

> 93:   double steepness = 6.0;
> 94: 
> 95:   return 1.0 / (1.0 + pow(M_E, -steepness * (value - inflection_point)));

Can use `exp` rather than `pow`.

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

> 163:   // Ensure the expansion size is at least the minimum growth amount
> 164:   // and at most the remaining uncommitted byte size.
> 165:   return clamp((size_t)resize_bytes, min_expand_bytes, uncommitted_bytes);

Don't need a cast of `resize_bytes` here.

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

> 300:     if (_g1h->capacity() == _g1h->max_capacity()) {
> 301:       log_resize(short_term_pause_time_ratio, long_term_pause_time_ratio,
> 302:                  lower_threshold, upper_threshold, pause_time_threshold, true, 0, expand);

We're on the expand-side of things, but `expand` hasn't been set to true yet.
Similarly, on the shrink side, we'll be passing in the default initial value for `expand` and
then setting it later. Maybe `expand` should be set earlier on each of the branches.

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

PR Review: https://git.openjdk.org/jdk/pull/25832#pullrequestreview-2935176410
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2152197151
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160408699
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160409675
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160411832
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160412727
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160414808
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160414939
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160416276
PR Review Comment: https://git.openjdk.org/jdk/pull/25832#discussion_r2160508087