RFR: 8356716: ZGC: Cleanup Uncommit Logic [v3]

[Joel Sikström]

On Tue, 20 May 2025 06:55:21 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> [JDK-8350441](https://bugs.openjdk.org/browse/JDK-8350441) required changing the way ZGC handle memory uncommitting (returning physical memory to the OS). Previously ZGC tracked how recently used memory was on a ZPage level. [JDK-8350441](https://bugs.openjdk.org/browse/JDK-8350441) did away with the ZPage abstraction for unused memory. But because of this ZGC does not have a convenient way of tracking the usage of a specific memory range. Instead [JDK-8350441](https://bugs.openjdk.org/browse/JDK-8350441) opted to keep a watermark in the cache unused mapped memory, to keep track of the amount of memory that was not used within the last ZUncommitDelay, and use this when deciding how much to uncommit.
>> 
>> Because this measurement is not as granular as previously, and because uncommitting memory is something we want to do conservatively, as a response to low memory utilization, [JDK-8350441](https://bugs.openjdk.org/browse/JDK-8350441) was written with the intent to spread out the uncommitting over some time interval.
>> 
>> The actual implementation in [JDK-8350441](https://bugs.openjdk.org/browse/JDK-8350441) has a few issues which this RFE tries to address:
>>   * Missing wait, the uncommitting is not actually spread out, but happens all at once.
>>   * Reactivity, if the process starts using memory that was below the previous watermark, uncommitting should stop.
>>   * Structure, the current implementation has a lot of different dependencies and has state spread out over multiple classes. Refactor to keep the logic contained to the ZUncommitter, and provide better named facilitating functions on the ZPartition and ZMappedCache. And make the lifecycle of ZUncommitter more explicit.
>>   * Events, overhaul the JFR uncommit events to be sent (and track the time for) a chunk of uncommits without any waits.
>> 
>> An alternative discussed has been to do uncommitting based on GC triggers rather than a periodically. So rather than using ZUncommitDelay, we could have our proactive GCs actually trigger and track uncommitting. This might be a future RFE, but it was not attempted here as it would change user facing APIs. [JDK-8329758](https://bugs.openjdk.org/browse/JDK-8329758) will more than likely overhaul the uncommit triggers as well, and the whole concept of ZUncommitDelay and having to tune how to uncommit will go away.
>
> Axel Boldt-Christmas has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream_jdk/master' into JDK-8356716
>  - Wrong too
>  - Less archaic spelling of complete
>  - Cleanup and simplify
>  - Move all uncommit logic to zUncommitter
>  - Log time spent uncommitting
>  - Split reset_uncommit_cycle and add headroom
>  - Rename _min_last_uncommit_cycle to _min_size_watermark
>  - Use milliseconds instead of seconds
>  - Improve events and statistics
>  - ... and 8 more: https://git.openjdk.org/jdk/compare/b3925eac...43c0795a

I like how uncommitting becomes more robust with this patch and the overall design of having an uncommit "cycle". Some thoughts:

I think we should move activation of the uncommit cycle to `ZUncommitter::run_thread()`, so that it is on the same "level/depth" that also deactivates it.

I'm not 100% sure of our style in ZGC, but since we're at it I think the functions in zUncommiter.cpp should match the order in the header, or the other way around.

src/hotspot/share/gc/z/zPhysicalMemoryManager.cpp line 119:

> 117:   if (ZUncommitDelay > max_delay_without_overflow) {
> 118:     FLAG_SET_ERGO(ZUncommitDelay, max_delay_without_overflow);
> 119:   }

This is a nice addition!

src/hotspot/share/gc/z/zUncommitter.cpp line 116:

> 114:         // Done
> 115:         break;
> 116:       }

Is it possible to convert this to something like the following to make it clearer that this is the "end condition" of the cycle? From what I can see, 2/3 paths that return 0 in `uncommit()` calls `cancel_uncommit_cycle()`.
Suggestion:

      if (uncommit_cycle_is_canceled() || uncommit_cycle_is_finished()) {
        // No more work, cycle is done.
        break;
      }

src/hotspot/share/gc/z/zUncommitter.cpp line 358:

> 356:       cancel_uncommit_cycle();
> 357:       return 0;
> 358:     }

Maybe?
Suggestion:

    if (limit == 0) {
      // This may occur if the current max capacity for this partition is 0
      cancel_uncommit_cycle();
      return 0;
    }

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

PR Review: https://git.openjdk.org/jdk/pull/25198#pullrequestreview-2868116856
PR Review Comment: https://git.openjdk.org/jdk/pull/25198#discussion_r2107304605
PR Review Comment: https://git.openjdk.org/jdk/pull/25198#discussion_r2107391145
PR Review Comment: https://git.openjdk.org/jdk/pull/25198#discussion_r2107129706