RFR: 8312117: GenShen: Preempt OLD marking more quickly when YOUNG triggers arise [v2]

[Y. Srinivas Ramakrishna]

On Mon, 2 Oct 2023 18:05:03 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Have regulator thread wait until expected condition is met
>
> William Kemper has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Use same precision for time checks as log messages
>  - Regulator lock should protect the write to mode

Looks good to me, although I have not yet fully digested the request-response synchronization skeleton fully yet.

Just left a few minor comments.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 911:

> 909: 
> 910:     MonitorLocker ml(&_regulator_lock, Mutex::_no_safepoint_check_flag);
> 911:     while (_mode == none) {

`gc_mode()` instead of `_mode`.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 922:

> 920:   if (preempt_old_marking(generation)) {
> 921:     log_info(gc)("Preempting old generation mark to allow %s GC", shenandoah_generation_name(generation));
> 922:     assert(_mode == servicing_old, "Cannot preempt old if old cycle isn't running.");

Is this a stable condition at the point of this check? (i.e. can its value be changed by concurrent activity between the test at line 920 and the assert at line 922?)

Also, for programming hygiene, I'd use the accessor method `gc_mode()` everywhere, instead of `_mode` when accessing its value. This allows for easy changes for insertion of assertions, tests, etc. in the accessor method if needed (as well as making debugging a tad easier). Avoiding naked accesses to fields is almost always a good idea.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 930:

> 928: 
> 929:     MonitorLocker ml(&_regulator_lock, Mutex::_no_safepoint_check_flag);
> 930:     while (_mode == servicing_old) {

`gc_mode()` instead of `_mode`.

src/hotspot/share/gc/shenandoah/shenandoahRegulatorThread.cpp line 150:

> 148: 
> 149: bool ShenandoahRegulatorThread::start_old_cycle() {
> 150:   // TODO: These first two checks might be vestigial

It would seem that not starting a fresh old marking cycle while we have copying work left to do from a previous one does however seem to be reasonable.

Why do you believe this is vestigial? Have we already checked these conditions elsewhere recently? (I realize we can't assert those two conditions since the state may have changed concurrently because of concurrent collection activity.)

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/333#pullrequestreview-1653270193
PR Review Comment: https://git.openjdk.org/shenandoah/pull/333#discussion_r1343049765
PR Review Comment: https://git.openjdk.org/shenandoah/pull/333#discussion_r1343042620
PR Review Comment: https://git.openjdk.org/shenandoah/pull/333#discussion_r1343050571
PR Review Comment: https://git.openjdk.org/shenandoah/pull/333#discussion_r1343017858