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

[William Kemper]

On Mon, 2 Oct 2023 19:06:53 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> 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
>
> src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 912:
> 
>> 910:     MonitorLocker ml(&_regulator_lock, Mutex::_no_safepoint_check_flag);
>> 911:     while (_mode == none) {
>> 912:       bool timeout = ml.wait(5);
> 
> Should this be a globals value? Are there other such "took too long to respond" loops governed by a timeout variable defined in shenandoah_globals.hpp that might be open to overloaded use here perhaps?

I can remove the timeout, I was just being paranoid. With the `regulator_lock` being used correctly, it shouldn't be necessary to have a timeout.

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

Yes, the `preempt_old_marking` condition can only happen during certain phases of an old cycle. If we are able to preempt old marking, then the controller thread _must_ be `servicing_old`.

I'll make changes to use the accessor method.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/333#discussion_r1343059607
PR Review Comment: https://git.openjdk.org/shenandoah/pull/333#discussion_r1343058643