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

[Monica Beckwith]

On Thu, 17 Jul 2025 07:01:07 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> 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
>
> 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 :) )

ok. will change as follows:

// pages were at least *requested* to be released → // Pages were at least requested to be released 

and 

// we *did* something → // We did something

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

I agree. I started with time-based since I wanted to have a catch-all term for log parsers. But since we also have the `sizing` tag now, I can change the log messages to something more meaningful like "Uncommit evaluation"

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

Great points. Will update to:


Ticks current_ticks = Ticks::now();
Tickspan elapsed = current_ticks - _last_access_ticks;
return elapsed > _uncommit_delay;

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2216480242
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2216496524
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2216439181