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

[Monica Beckwith]

On Tue, 2 Sep 2025 10:17:33 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
>
> Also, garbage collections and the `VM_G1ShrinkHeap` operation need to reset the timestamp for all (free) regions too. Otherwise the mechanism will try to uncommit the wrong amount of regions, similar to the issue I described in `VM_G1ShrinkHeap`.
> 
> E.g. Regions A, B, C are free for a long time, GC happens that keeps A, B, C for various reasons, now idle-evaluation happens, and A, B, C are determined idle in addition to the other regions if their timestamp is not reset. So the idle uncommit will free too many regions.

Hi @tschatzl 

Following up on my December reply with additional fixes and performance verification.

## Fixes

| Issue | File | Status |
|-------|------|--------|
| O(1) timestamp reset after GC | g1HeapRegionManager | Added `_last_gc_timestamp`; `reset_free_region_timestamps()` called after young/full GC; uses `MAX2(region_time, _last_gc_time)` |
| `_last_access_timestamp(Ticks::now())` in constructor | g1HeapRegion.cpp | Default-initialized (0); set when region transitions to free |
| `record_activity()` call in hr_clear() | g1HeapRegion.cpp | Removed - `set_free()` handles timestamp update |
| `record_activity()` method | g1HeapRegion.hpp | Renamed to `update_last_access_timestamp()` |
| Unused `should_uncommit()` method | g1HeapRegion.hpp | Removed |
| Unused `deactivate_region_at()` method | g1CollectedHeap.hpp | Removed |
| `_heap_evaluation_task` not in initializer list | g1CollectedHeap.cpp | Moved to constructor initializer list |
| Unnecessary assert about `_heap_evaluation_task` | g1CollectedHeap.cpp | Removed |
| Unnecessary init log when registering task | g1CollectedHeap.cpp | Removed (G1ServiceThread handles it) |
| `request_heap_shrink()` return value never used | g1CollectedHeap | Changed to `void` |
| `total_regions` variable unused | g1HeapSizingPolicy.cpp | Removed |
| `current_heap` naming unclear | g1HeapSizingPolicy.cpp | Renamed to `current_capacity` |
| `_analytics` could be nullptr | g1HeapSizingPolicy.cpp | Added `assert(_analytics != nullptr)` in constructor |
| G1ReservePercent calculation adds instead of MAX | g1HeapSizingPolicy.cpp | Uses `MAX2(g1_reserve_regions, young_gen_regions)` |
| ResourceMark unnecessary | g1HeapSizingPolicy.cpp | Removed |
| Duplicate log messages | g1HeapEvaluationTask.cpp | Consolidated info/debug into single log |
| Manual iteration instead of `heap_region_iterate()` | g1HeapRegionManager.cpp | Uses closure with `iterate()` |
| `current_time` should be taken once | g1HeapRegionManager.cpp | Taken before iteration |

## Performance Verification

SPECjbb2015 benchmarking (4 iterations per configuration) across 5 TBS configurations varying `G1UncommitDelayMillis` and `G1TimeBasedEvaluationIntervalMillis`. No performance regression when compared to baseline (default).

## Lower Priority Items

Happy to address if you prefer:
- Task naming: `G1HeapEvaluationTask` → `G1TimeBasedHeapEvaluationTask`
- Log context: Add more detail at info level

Regarding log tags: I'd prefer to keep `gc,sizing` as TBS is a distinct feature - it's not STW or concurrent GC, just memory reclaim. The `sizing` tag helps users isolate TBS behavior from traditional GC logging.

Looking forward to your thoughts.

Thanks,
Monica

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

PR Comment: https://git.openjdk.org/jdk/pull/26240#issuecomment-3857709271