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

[Monica Beckwith]

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

> I stopped reviewing after >40 issues (still finding some while writing) because
> 
> * some of them were unresolved from last time. Please resolve comments that have been addressed or comment why the change did not address it. Having old unresolved comments are annoying for re-reviews because they make the reviewer unsure if there is some need for further clarification or not.
> * new code introduced the same issues again (e.g. use of `is_empty()`) without comment on why it can be used, after giving the reason why it can't be used.
> * the main issue that the time-based uncommit still interferes with gc based sizing without restriction still applies.
> * another is that the VM operation still does not re-evaluate the decisions, just uncommits a set number of regions. Possibly not even those that were found as candidates earlier afaict.
> * the only improvement I remember has been that `ServiceTask` is used now

Hi @tschatzl,

Thank you for the comprehensive review! I've addressed all the issues you raised in reviews. Since I cannot close comments that I didn't open, I've been checking them off as I completed each fix.

## Major Architectural Fix

**VM Operation Re-evaluation**

You were absolutely right that the service thread evaluation and VM operation execution were disconnected. The region selection was happening outside the safepoint, which could lead to incorrect behavior during concurrent GC operations.

**Fixed:** Modified `VM_G1ShrinkHeap::doit()` to re-evaluate uncommit candidates at the safepoint using `find_uncommit_candidates_by_time()`. This ensures we validate that candidate regions are still free at the point of uncommit:


void VM_G1ShrinkHeap::doit() {
  uint max_regions_to_shrink = (uint)(_bytes / G1HeapRegion::GrainBytes);
  GrowableArray<G1HeapRegion*> candidates(max_regions_to_shrink);
  _g1h->heap_sizing_policy()->find_uncommit_candidates_by_time(&candidates, max_regions_to_shrink);
  
  // Validation loop ensures regions are still free before uncommitting
  _g1h->shrink_with_time_based_selection(shrink_bytes);
}


## Code Quality Improvements

**Redundant `is_empty()` Checks**

You pointed out that `is_empty() && is_free()` was redundant since `is_free()` already implies the region is empty.

**Fixed:** Removed all redundant checks in g1HeapRegionManager.cpp and g1HeapSizingPolicy.cpp. Now using only `hr->is_free()` which is the proper check.

**Bubble Sort → qsort**

Good catch on the inefficient bubble sort implementation.

**Fixed:** Replaced with `GrowableArray::sort()` using a lambda comparator in g1HeapRegionManager.cpp. This provides O(n log n) performance and leverages the standard library implementation:


static auto compare_region_age = [](G1HeapRegion** a, G1HeapRegion** b) -> int {
  Ticks time_a = (*a)->last_access_time();
  Ticks time_b = (*b)->last_access_time();
  if (time_a < time_b) return -1;
  if (time_a > time_b) return 1;
  return 0;
};
empty_regions.sort(compare_region_age);


**Comment Punctuation**

**Fixed:** Added periods to all sentence-style comments throughout the modified files for consistency with OpenJDK style guidelines.

**ResourceMark Comment**

**Fixed:** Removed the unclear "ResourceMark rm" comment in G1HeapEvaluationTask::execute() as requested.

**GC Alloc Regions Validation**

**Fixed:** Changed abandon_gc_alloc_regions() to an assertion checking they're already abandoned, rather than doing unnecessary work.

**GCCause Assertion**

**Fixed:** Added assertion that GC cause is _no_gc when performing time-based uncommit.

**Duplicate Log Information**

**Fixed:** Consolidated log messages to avoid repeating same information at different log levels. Lower level log now contains superset of information.

**Redundant idle_count Check**

**Fixed:** Removed redundant check for `idle_count < G1MinRegionsToUncommit` that was already validated at entry condition.

**Unnecessary Validation Method**

**Fixed:** Removed unnecessary validation method since candidates are already validated when selected with is_free() check.

## Already Correct

**SuspendibleThreadSetJoiner**

You mentioned this should be in the service task, not the VM operation. The implementation was already correct - the `SuspendibleThreadSetJoiner` is in `G1HeapEvaluationTask::execute()`, which runs on the service thread. The VM operation only handles the actual uncommit at the safepoint.

**GC Coordination**

The implementation already ensures GC-based sizing takes precedence through several mechanisms:
- The evaluation only proceeds when `short_term_gc_time_ratio() < 0.05` (5%)
- Time-based uncommit respects G1's existing reserve calculation (G1ReservePercent, default 10%)
- Young generation requirements are always preserved
- The conservative reserve ensures quick re-commit isn't needed during allocation bursts

**Assertions and Validations**

The code already includes proper assertions for GC cause validation and task initialization checks as suggested. The Heap_lock assertion was updated to allow safepoint execution: `assert(Heap_lock->owner() != nullptr || SafepointSynchronize::is_at_safepoint(), ...)`.

Let me know if you have any questions about these changes or if there's anything else you'd like me to address!

Thanks.

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

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