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

[Aleksey Shipilev]

On Mon, 11 Nov 2024 17:31:58 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.

Cursory review:

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 91:

> 89: #include "runtime/stackWatermarkSet.hpp"
> 90: #include "runtime/vmThread.hpp"
> 91: #include "gc/shenandoah/shenandoahUncommitThread.hpp"

Includes are usually in alpbabetical order :)

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1345:

> 1343:   if (_uncommit_thread != nullptr) {
> 1344:     tcl->do_thread(_uncommit_thread);
> 1345:   }

New line after new block?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1932:

> 1930:   if (_uncommit_thread != nullptr) {
> 1931:     _uncommit_thread->stop();
> 1932:   }

Are there limits on proper sequencing here? Can we shutdown uncommit thread before cancelling the GC and waiting for control thread to exit? This would save end-to-end time for short commands, as we would hide the uncommit thread shutdown in the shadow of control thread exiting.

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

> 53: 
> 54:     if (soft_max_changed || explicit_gc_requested || current - last_shrink_time > shrink_period) {
> 55:       double shrink_before = (soft_max_changed || explicit_gc_requested) ? current : current - ((double) ShenandoahUncommitDelay / 1000.0);

Suggestion:

      double shrink_before = (soft_max_changed || explicit_gc_requested) ? 
         current : 
         current - ((double) ShenandoahUncommitDelay / 1000.0);

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

> 67:       MonitorLocker locker(&_lock, Mutex::_no_safepoint_check_flag);
> 68:       if (!_stop_requested.is_set()) {
> 69:         locker.wait((int64_t )shrink_period);

Suggestion:

        locker.wait((int64_t)shrink_period);

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

> 137:     _heap->notify_heap_changed();
> 138:     double elapsed = os::elapsedTime() - start;
> 139:     log_info(gc)("Uncommitted " SIZE_FORMAT " regions, in %.3fs", count, elapsed);

If we can, can we match the current log format? E.g. print `Concurrent uncommit`, with appropriately formatted timestamp? I think we also want `log_info(gc,start)` at the beginning of the method. I think `ShenandoahConcurrentPhase` helper did all that, can we still use it?

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.hpp line 37:

> 35:   ShenandoahSharedFlag _explicit_gc_requested;
> 36:   ShenandoahSharedFlag _stop_requested;
> 37:   Monitor _lock;

Which one of these can be `const`?

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

PR Review: https://git.openjdk.org/jdk/pull/22019#pullrequestreview-2430298661
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1838524990
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1838527316
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1838530829
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1838568788
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1838569201
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1838537328
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1838569638