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

[Thomas Schatzl]

On Wed, 27 Aug 2025 11:57:45 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/g1HeapRegion.hpp line 564:
> 
>> 562:   void record_activity() {
>> 563:     _last_access_timestamp = Ticks::now();
>> 564:   }
> 
> Does not seem to be used outside `G1HeapRegion`. Remove, or better just inline since it only seems to be used once. Or put the implementation into the cpp file (and make `private`).

Since my comments make this call single-use, the change can inline it.

> src/hotspot/share/gc/g1/g1HeapRegion.hpp line 578:
> 
>> 576:     Tickspan elapsed = current_time - _last_access_timestamp;
>> 577:     return elapsed > delay;
>> 578:   }
> 
> Does not seem to be used? In general, I would tend to avoid putting policy stuff into `G1HeapRegion` which should just be a container for region state members. Also, this method is too complex imho to put into the `.hpp` file directly (should be in `.inline.hpp`), but since this method should be removed anyway....

Also still unused.

> src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 488:
> 
>> 486: bool G1HeapSizingPolicy::should_uncommit_region(G1HeapRegion* hr) const {
>> 487:   // Note: Caller already guarantees hr->is_empty() is true
>> 488:   // Empty regions should always be free and not in collection set in normal operation
> 
> Sentences should end with a full stop in comments (I am stopping commenting on this here, but there are quite a few of those).

Also not fixed? I commented it again in another place....

> src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 521:
> 
>> 519:   // Must hold Heap_lock during heap resizing
>> 520:   MutexLocker ml(Heap_lock);
>> 521: 
> 
> Since this can be entered right after a garbage collection, maybe the evaluation should be immediately exited in that case? Garbage collection already found the "right" heap size, no need for idle calculation.
> Otoh, `get_uncommit_candidates()` should find nothing in this case.

Not sure if this comment is still relevant or has been addressed. See above for basically the same comment (afaiu).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2546144314
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2546146548
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2546173161
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2546260963