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

[Thomas Schatzl]

On Thu, 20 Nov 2025 12:12:44 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Monica Beckwith has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Fix compilation errors after master merge
>>    
>>    - Fix syntax error in g1CollectedHeap.cpp (extra closing brace)
>>    - Update API call from short_term_pause_time_ratio() to short_term_gc_time_ratio()
>>    
>>    These fixes resolve compilation issues that occurred after merging with
>>    upstream master due to API changes in the OpenJDK codebase.
>>  - 8357445: Address feedback for G1 time-based heap sizing
>>    
>>    - Fix indentation in log_debug statement in shrink_helper (suggested by @tschatzl)
>>    - Change terminology from 'inactive' to 'idle' throughout time-based heap sizing
>>    - Update flag descriptions in g1_globals.hpp to use 'idle' terminology
>>    - Fix remaining trailing whitespace in test files
>>    
>>    Addresses all outstanding review comments from @tschatzl
>
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1122:
> 
>> 1120:   // We should only reach here from the service thread during idle time
>> 1121:   // but ensure any GC alloc regions are abandoned
>> 1122:   _allocator->abandon_gc_alloc_regions();
> 
> Maybe check that they are abandoned (i.e. there are no gc alloc regions) in an assert instead of doing work.

It's probably best to check that the current GC cause is `GCCause::_no_gc` (and use the `GCCauseSetter` in the VM operation) in an assert here.
Maybe it's worth making an extra `GCCause` for this, but right now I do not see a reason. As far as I can tell, the default cause is `_no_gc` anyway... (so actually nothing to do in the VM operation).

> src/hotspot/share/gc/g1/g1HeapEvaluationTask.cpp line 60:
> 
>> 58: 
>> 59:   if (resize_amount > 0) {
>> 60:     log_info(gc, sizing)("Uncommit evaluation: shrinking heap by %zuMB using time-based selection", resize_amount / M);
> 
> At least at info level, more context is needed.

Other tasks use the `gc, task` tags for such status messages afaict.

> src/hotspot/share/gc/g1/g1HeapRegionManager.cpp line 686:
> 
>> 684:     shrink_at(region_index, 1);
>> 685:     removed++;
>> 686:   }
> 
> I think I need to see this policy in action, blindly removing all too-old regions seems to be too severe an action. 
> 
> These free regions are also needed for the young gen + old gen, which has been sized appropriately. It should probably at least take them into account.

Also the code *must* reset the timestamp for regions not selected for the reasons stated in another comment.

> src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 535:
> 
>> 533: 
>> 534:     // Only count if region is ready for shrinking
>> 535:     if (hr->is_empty() && hr->is_free()) {
> 
> `is_empty()` is wrong, already mentioned this in the last review.

All free regions are empty.

> src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 641:
> 
>> 639:       // This prevents expensive re-commits during the next GC or allocation burst
>> 640:       // We add G1MinRegionsToUncommit as a small safety buffer beyond G1's standard reserves
>> 641:       size_t min_regions_after_uncommit = reserved_regions + G1MinRegionsToUncommit;
> 
> The comment would rather indicate a `MAX2` use.... the name of the variable also indicates that this is a minimum regions one would want to uncommit, not an additional buffer.

Also, do you have evidence that this additional buffer is actually required?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2545849232
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2545946650
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2546167399
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2546407974
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2546402918