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

Wed Jul 16 18:46:47 UTC 2025

On Thu, 10 Jul 2025 18:09:17 GMT, Monica Beckwith <mbeckwit at openjdk.org> wrote:

>> **Implements:** https://bugs.openjdk.org/browse/JDK-8357445
>> 
>> Implement time-based heap uncommit for G1 during idle periods.
>> 
>> Key changes:
>> - Added G1HeapEvaluationTask for periodic heap evaluation
>> - Switch from G1ServiceTask to PeriodicTask for improved scheduling
>> - Implemented time-based heap sizing policy with configurable uncommit delay
>> - Added region activity tracking with last access timestamps
>> - Integrated VM_G1ShrinkHeap operation for safe heap shrinking
>> - Added new G1 flags: G1UseTimeBasedHeapSizing, G1TimeBasedEvaluationIntervalMillis, G1UncommitDelayMillis, G1MinRegionsToUncommit
>> - Added 'sizing' log tag for heap sizing operations
>> 
>> Comprehensive Test Coverage:
>> - Enhanced TestG1RegionUncommit: minimum heap boundaries, concurrent allocation/uncommit scenarios
>> - Enhanced TestTimeBasedHeapSizing: humongous object handling, rapid allocation cycles, edge cases
>> - Enhanced TestTimeBasedRegionTracking: concurrent region access, lifecycle transition validation
>> - Enhanced TestTimeBasedHeapConfig: parameter boundary values, small heap configurations
>> 
>> This ensures time-based heap uncommit works correctly while maintaining all safety guarantees and test expectations.
>
> Monica Beckwith has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Normalize encoding and line endings for G1 GC time-based tests

Changes requested by iwalulya (Reviewer).

src/hotspot/share/gc/g1/g1Allocator.hpp line 35:

> 33: class G1EvacInfo;
> 34: class G1NUMA;
> 35: class G1HeapSizingPolicy;

Probably not required here

src/hotspot/share/gc/g1/g1Allocator.inline.hpp line 61:

> 59:   }
> 60: 
> 61:   return result;

If this needs to be made, it should be done as a separate cleanup as it is not related to the CR.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1:

> 1: 

Extra line should be removed

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 43:

> 41: #include "gc/g1/g1ConcurrentRefineThread.hpp"
> 42: #include "gc/g1/g1HeapSizingPolicy.hpp"  // Include this first to avoid include cycle
> 43: #include "gc/g1/g1HeapEvaluationTask.hpp"

cycles cannot happen because we have the `#ifndef` for the .hpp files

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1078:

> 1076:                            shrink_bytes, shrunk_bytes, num_regions_removed);
> 1077:   if (num_regions_removed > 0) {
> 1078:     log_info(gc, heap)("Heap shrink completed: uncommitted %u regions (%zuMB), heap size now %zuMB",

At this point, we have flagged regions for uncommitting, but no guarantee that these regions have been uncommitted. The log entry might be confusing if we indicate that shrinking is completed, and yet NMT indicates otherwise.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1511:

> 1509:     log_debug(gc, init)("G1 Time-Based Heap Evaluation task created (PeriodicTask)");
> 1510:   } else {
> 1511:     _heap_evaluation_task = nullptr;

maybe replace with an `assert(_heap_evaluation_task == nullptr, "pre-condition");`

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1511:

> 1509:     log_debug(gc, init)("G1 Time-Based Heap Evaluation task created (PeriodicTask)");
> 1510:   } else {
> 1511:     _heap_evaluation_task = nullptr;

Suggestion:

    assert(_heap_evaluation_task == nullptr, "pre-condition")

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1575:

> 1573: 
> 1574:   // Enroll the heap evaluation task after G1 is fully initialized
> 1575:   if (G1UseTimeBasedHeapSizing && _heap_evaluation_task != nullptr) {

any reason why `_heap_evaluation_task == nullptr` could be `true` if `G1UseTimeBasedHeapSizing` is `true`?

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2962:

> 2960:   assert(alloc_region->is_eden(), "all mutator alloc regions should be eden");
> 2961: 
> 2962:   alloc_region->record_activity();  // Record the activity of the alloc region

Since we only uncommit free regions, we only need to `record_activity` when the region is freed which is done in `G1HeapRegion::hr_clear` before the region is added to the free-list

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 1:

> 1: 

Extra black line, please delete.

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 931:

> 929:   G1Policy* policy() const { return _policy; }
> 930:   // The heap sizing policy.
> 931:   G1HeapSizingPolicy* heap_sizing_policy() const { return _heap_sizing_policy; }

unused method, probably not needed.

src/hotspot/share/gc/g1/g1HeapEvaluationTask.cpp line 36:

> 34: #include "utilities/globalDefinitions.hpp"
> 35: 
> 36: G1HeapEvaluationTask::G1HeapEvaluationTask(G1CollectedHeap* g1h, G1HeapSizingPolicy* heap_sizing_policy) :

`G1HeapEvaluationTask` class name does not seem to clearly state the purpose of the class. But I cannot come up with a better name.

src/hotspot/share/gc/g1/g1HeapEvaluationTask.cpp line 44:

> 42: void G1HeapEvaluationTask::task() {
> 43:   // This runs on WatcherThread during idle periods - perfect for time-based evaluation!
> 44:   log_debug(gc, sizing)("Starting heap evaluation");

For existing Heap Resizing tasks, we use the tags `(gc, ergo, heap)`, I would suggest we maintain that.

src/hotspot/share/gc/g1/g1HeapEvaluationTask.hpp line 35:

> 33: 
> 34: // Time-based heap evaluation task that runs during idle periods
> 35: class G1HeapEvaluationTask : public PeriodicTask {  // Changed from G1ServiceTask

Add a comment explaining why we use `PeriodicTask`  instead of  `G1ServiceTask`

Then the comments on ` // Changed from g1ServiceThread.hpp` and `// Changed from execute() to task()` will not be required

src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp line 46:

> 44: void G1HeapSizingPolicy::initialize() {
> 45:   // Flag values are available at this point
> 46:   _uncommit_delay_ms = (jlong)G1UncommitDelayMillis;

I think we can "safely" do this in the constructor for G1HeapSizingPolicy, then we don't need the `initialize` method. 

We also do not need to make `_uncommit_delay_ms`, `G1HeapSizingPolicy` relies on the singleton pattern through `G1HeapSizingPolicy::create`.

src/hotspot/share/runtime/vmOperations.cpp line 649:

> 647: }
> 648: #endif // INCLUDE_G1GC
> 649: 

Please move to `g1VMOperations.cpp`

src/hotspot/share/runtime/vmOperations.hpp line 308:

> 306:   bool is_gc_operation() const override { return true; }
> 307:   void doit() override;
> 308: };

Should be moved to `g1VMOperations.hpp`

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

PR Review: https://git.openjdk.org/jdk/pull/26240#pullrequestreview-3025735357
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210888250
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210859827
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210910763
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210862304
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210920641
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211025677
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211027979
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211045517
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211237408
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210904505
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210908900
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211257389
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211254835
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2210937453
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211010976
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211278259
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211268097