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

[Monica Beckwith]

On Wed, 16 Jul 2025 16:16:19 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> 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
>
> src/hotspot/share/gc/g1/g1Allocator.hpp line 35:
> 
>> 33: class G1EvacInfo;
>> 34: class G1NUMA;
>> 35: class G1HeapSizingPolicy;
> 
> Probably not required here

removed

> 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.

sounds good, will revert.

> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1:
> 
>> 1: 
> 
> Extra line should be removed

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

fixed and removed comment

> 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.

changed to "flagged"

> 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");`

replaced with assert

> 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`?

fixed - when G1UseTimeBasedHeapSizing is true, _heap_evaluation_task is created in the constructor, so it should never be nullptr in post_initialize()

> src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 1:
> 
>> 1: 
> 
> Extra black line, please delete.

removed

> 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.

removed

> 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

done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211670618
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211671247
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211671414
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211671927
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211672028
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211672215
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211674671
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211674803
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211674926
PR Review Comment: https://git.openjdk.org/jdk/pull/26240#discussion_r2211675423