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

[Y. Srinivas Ramakrishna]

On Fri, 15 Nov 2024 21:35:03 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 incrementally with one additional commit since the last revision:
> 
>   Prevent uncommit thread from running during GC

Looks good to me. A few documentation comment requests.

Also please share performance data in this PR or in the ticket, especially from the perf/benchmark that may have precipitated this change.

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 214:

> 212: //
> 213: public:
> 214:   void notify_heap_changed();

Let's place a single line of documentation comment for all the public and private APIs at lines that we touch in a PR where documentation is missing. (I realize you merely changed the method from private to public.)

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 406:

> 404:   void notify_soft_max_changed();
> 405:   void notify_explicit_gc_requested();
> 406: 

1-line documentation of API.

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 411:

> 409: private:
> 410:   ShenandoahControlThread*   _control_thread;
> 411:   ShenandoahUncommitThread*  _uncommit_thread;

Role of thread, e.g.

.... // a thread to uncommit selected free regions of the heap

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 628:

> 626:   bool is_uncommit_in_progress();
> 627: #endif
> 628: 

1-line API documentation each.

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

> 85:   // Determine if there is work to do. This avoids taking heap lock if there is
> 86:   // no work available, avoids spamming logs with superfluous logging messages,
> 87:   // and minimises the amount of work while locks are taken.

last word: taken -> held.

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

> 38:   ShenandoahSharedFlag _uncommit_in_progress;
> 39:   Monitor _stop_lock;
> 40:   Monitor _uncommit_lock;

A 1-line comment on role of each field, e.g.

ShenandoahSharedFlag _soft_max_changed;    // the heap's soft max target has changed recently

etc.

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

> 44:   void uncommit(double shrink_before, size_t shrink_until);
> 45: 
> 46:   bool is_uncommit_allowed();

Would be nice to document these private methods as well.

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

Marked as reviewed by ysr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22019#pullrequestreview-2444049683
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1847475867
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1847477020
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1847479562
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1847480153
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1847516208
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1847519410
PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1847508283