RFR: 8357445: G1: Time-Based Heap Uncommit During Idle Periods [v7]

[Thomas Schatzl]

On Wed, 27 Aug 2025 11:20:36 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Monica Beckwith has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove unused static _uncommit_delay member and accessor
>
> src/hotspot/share/gc/g1/g1HeapEvaluationTask.cpp line 54:
> 
>> 52:     log_trace(gc, sizing)("GC active, skipping uncommit evaluation");
>> 53:     return;
>> 54:   }
> 
> This is sketchy. Unless you manually sync, there is no guarantee this is valid. Also since the code is running in a separate thread that might not be stopped during GC, the GC VM operation might just start after this check.

I.e. use `SuspendibleThreadSetJoiner` etc.

> src/hotspot/share/gc/g1/g1HeapRegion.cpp line 253:
> 
>> 251:   _age_index(G1SurvRateGroup::InvalidAgeIndex),
>> 252:   _node_index(G1NUMA::UnknownNodeIndex),
>> 253:   _last_access_timestamp(Ticks::now()), // Initialize timestamp with current time
> 
> Since `G1HeapRegion::initialize()` is called on this region just a bit later, I recommend not wasting the time to call `Ticks::now()` for no gain. Just initialize to a high value(?) and things should work out I think if the code ever comes across such a badly initialized method.

Fwiw, to avoid startup delays (calling `Ticks::now()` on thousands of regions is expensive - I did not test, but it seems reasonable and should be measured), it might be useful to do that initialization lazily (first time the background thread executes).

> src/hotspot/share/gc/g1/g1HeapRegion.hpp line 257:
> 
>> 255:   uint _node_index;
>> 256: 
>> 257:   // Time-based heap sizing: tracks last allocation/access time
> 
> Suggestion:
> 
>   // Time-based heap sizing: tracks last allocation/access time.

The comment is wrong/outdated too, only tracks last freeing (clearing) time. Maybe there is a better name than `_last_access_time` for the member too.

> src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 550:
> 
>> 548:       // 1. No more than 25% of inactive regions
>> 549:       // 2. No more than 10% of total committed regions
>> 550:       // 3. No more than max_shrink_bytes worth of regions
> 
> This policy sounds like `Min/MaxHeapFreeRatio` that are known to be not useful. Do not have a better idea right now either. Maybe look at what ZGC did at some point.

I also do not think this mechanism should ever uncommit the free regions reserved for the current young gen (without GC/recalculating the needed young gen) and potentially the survivor/old regions. There does not seem to be a limit here.

Otherwise the next GC (or just continuing work) will be riddled with in-pause commits. Or it will probably do a premature GC, I did not check.

Maybe this change should only madvise these regions as MADV_DONTNEED? (Not sure it does anything though).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2303669369
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2303701876
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2303714473
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2315538730