RFR: 8342444: Shenandoah: Uncommit regions from a separate, STS aware thread [v8]

[Aleksey Shipilev]

On Tue, 26 Nov 2024 01:07:39 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Currently, Shenandoah uncommits regions from its control thread. The control thread is responsible for starting GC cycles in a timely fashion. Uncommitting memory from this thread may introduce unwanted delays in the control thread's response to GC pressure.
>
> William Kemper 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:
> 
>  - Log uncommitted delta and capacity
>  - Merge remote-tracking branch 'jdk/master' into shen-uncommit-thread
>  - Restore logging format, show change in committed heap, rather than usage
>  - Allow commits initially
>  - Use idiomatic name for CADR class
>  - Improve comments
>  - Do not notify uncommit thread when uncommit is forbidden
>  - Prevent uncommit thread from running during GC
>  - Style and formatting fixes
>  - Alphabetize includes in shenandoahHeap.cpp
>  - ... and 8 more: https://git.openjdk.org/jdk/compare/a518ae81...847a2593

Changes requested by shade (Reviewer).

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp line 75:

> 73:       MonitorLocker locker(&_stop_lock, Mutex::_no_safepoint_check_flag);
> 74:       if (!_stop_requested.is_set()) {
> 75:         locker.wait((int64_t)shrink_period);

I tried to test this on some of my toy examples, and realized this particular line may end up as `locker.wait(0)`, which means "wait indefinitely, until notified". This breaks periodic commits. The old code rode on control thread doing `MAX2(1, ...)`, so we never feed `0` into `wait`. I am also confused about units. The comment above says `shrink_period` is in seconds, but `locker.wait` accepts milliseconds?

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp line 181:

> 179:   log_info(gc)("%s " PROPERFMT "(" PROPERFMT ") %.3fms",
> 180:                msg, PROPERFMTARGS(committed_start - committed_end), PROPERFMTARGS(_heap->capacity()),
> 181:                elapsed * MILLIUNITS);

I think we want an additional space. I see the current output is:


[11.366s][info][gc] Concurrent uncommit 32768K(192M) 2.506ms


Should probably be:


[11.366s][info][gc] Concurrent uncommit 32768K (192M) 2.506ms

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

PR Review: https://git.openjdk.org/jdk/pull/22019#pullrequestreview-2461003962
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1858191192
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1858203087