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

[Aleksey Shipilev]

On Tue, 26 Nov 2024 10:10:42 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

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

It sounds like this line should be:


        locker.wait(MAX2<int64_t>(1, shrink_period * 1000));

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1858200969