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

[Thomas Schatzl]

On Thu, 17 Jul 2025 01:50:59 GMT, Monica Beckwith <mbeckwit at openjdk.org> wrote:

>> **Implements:** https://bugs.openjdk.org/browse/JDK-8357445
>> 
>> Implement time-based heap uncommit for G1 during idle periods.
>> 
>> Key changes:
>> - Added G1HeapEvaluationTask for periodic heap evaluation
>> - Switch from G1ServiceTask to PeriodicTask for improved scheduling
>> - Implemented time-based heap sizing policy with configurable uncommit delay
>> - Added region activity tracking with last access timestamps
>> - Integrated VM_G1ShrinkHeap operation for safe heap shrinking
>> - Added new G1 flags: G1UseTimeBasedHeapSizing, G1TimeBasedEvaluationIntervalMillis, G1UncommitDelayMillis, G1MinRegionsToUncommit
>> - Added 'sizing' log tag for heap sizing operations
>> 
>> Comprehensive Test Coverage:
>> - Enhanced TestG1RegionUncommit: minimum heap boundaries, concurrent allocation/uncommit scenarios
>> - Enhanced TestTimeBasedHeapSizing: humongous object handling, rapid allocation cycles, edge cases
>> - Enhanced TestTimeBasedRegionTracking: concurrent region access, lifecycle transition validation
>> - Enhanced TestTimeBasedHeapConfig: parameter boundary values, small heap configurations
>> 
>> This ensures time-based heap uncommit works correctly while maintaining all safety guarantees and test expectations.
>
> Monica Beckwith has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8357445: Remove redundant record_activity calls and leftover initialize call
>   
>   - Remove record_activity() from retirement methods as hr_clear() is always last
>   - Remove leftover initialize() call since initialization moved to constructor
>   - Remove unused G1 includes from vmOperations after moving VM_G1ShrinkHeap

Some initial drive-by comments...

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

> 1134:   if (SafepointSynchronize::is_at_safepoint()) {
> 1135:     shrink(shrink_bytes);
> 1136:     return true;                     // we *did* something

(Full) Sentence length comments should follow sentence style: Initial capitalization and full stop at the end. Not sure if the emphasis should be kept, seems to be overused (and I do not think we use it a lot in G1 code).
(Please no markdown btw :) )

src/hotspot/share/gc/g1/g1HeapEvaluationTask.cpp line 64:

> 62:     // Time-based evaluation only handles uncommit/shrinking, never expansion
> 63:     if (should_expand) {
> 64:       log_warning(gc, sizing)("Time-based evaluation unexpected expansion request ignored (resize_amount=%zuB)", resize_amount);

I think "Time-based evaluation" as a kind of extra tag seems to be too unspecific: Time based evaluation of what?

I.e. the heap sizing based on GCTimeRatio is also "time-based". Also the messages partially seem to be inconsistent at first glance: Somebody this "time-based" prefix is there, sometimes not. Also sometimes it reads "Time-based evaluation", sometimes "Time-based heap sizing", sometimes having ":" in them, sometimes not.

If we add an extra tag, that "Time-based*" prefix might be superfluous, particularly if we keep the `sizing`for this kind of stuff (or even make it more specific).

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

> 542:     size_t max_shrink_bytes = current_heap > min_heap ? current_heap - min_heap : 0;
> 543: 
> 544:     log_trace(gc, sizing)("Time-based evaluation details: current_heap=%zuB min_heap=%zuB "

I do not think `trace`log messages need the "details" in `Time-based evaluation details" - this is already indicated by the log level.

Also, in contrast to other "Time-based evaluation" messages this has a ":" at the end, which is inconsistent.

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

> 72: //
> 73: class G1HeapSizingPolicy: public CHeapObj<mtGC> {
> 74:   static jlong _uncommit_delay_ms;  // Delay before uncommitting inactive regions

It's generally better to use `Ticks`/`Timespan` etc for times as they a) have higher precision and b) one does not need to carry the unit in them. Just convert when you need. They are also distinct types so that wrong assignment can not happen (like a value containing seconds to something that ought to contain milliseconds).
One can only round down etc. when needed.

Also, `jlong` should be avoided, as it is originally a "J"ava type, i.e. for Java interfacing (yeah, it has crept in over time for various reasons).

src/hotspot/share/gc/g1/g1_globals.hpp line 391:

> 389:           "attempt to uncommit memory")                                     \
> 390:           range(1, max_uintx)                                               \
> 391:                                                                             \

Just an initial drive-by comment:
* not sure I would make `G1UseTimeBasedHeapSizing` experimental (_maybe_ G1MinRegionsToUncommit`, but it seems very similar in use), and disabled by default. I would enable it by default, and make it diagnostic. While the difference between experimental and diagnostic is somewhat arguable, the former is more for developers to enable/disable, and the latter for support team to handle support issues (i.e. diagnose them).
As for the default value, I expect this to be beneficial always, and the default value indicates a long enough default delay for doing that, i.e. the overhead should be negligible.
* new manageble flags need a CSR to officially announce the functionality (and the feature needs a release note). Probably also tuning guide changes, but since it is not editable by the public, we at Oracle will handle this.
I am not yet completely convinced to make these manageable, although I can imagine some reasons. Can you give a good use case you specifically encountered to do so?

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

PR Review: https://git.openjdk.org/jdk/pull/26240#pullrequestreview-3028172712
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2212461687
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2212467046
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2212478262
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2212469596
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2212456893