RFR: 8321816: GenShen: Provide a minimum amount of time for an old collection to run [v4]

[William Kemper]

On Thu, 21 Dec 2023 03:08:39 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahRegulatorThread.cpp line 67:
>> 
>>> 65: }
>>> 66: 
>>> 67: void ShenandoahRegulatorThread::regulate_young_and_old_cycles() {
>> 
>> I am wondering if it might not be simpler and clearer to write this method as:
>> 
>> 
>> ...
>> while (!should_terminate()) {
>>   switch (mode) {
>>      case SCT::none : {
>>         // no collection is in progress
>>         // 1. check and start a global collection if needed
>>         if (should_unload_classes() && start_global_cycle()) {
>>           log_info...
>>         } else if (start_old_cycle()) {
>>            // 2. else check and start an old cycle
>>            log_info...
>>         } else if (start_young_cycle()) {
>>             // 3. else check and start a young cycle
>>            log_info...
>>         }
>>         break;
>>      }
>>      case SCT::servicing_old: {
>>          // if an old collection is in progress, check if a young
>>          // collection is needed. We can't start a global collection
>>          // in this state, even if one is needed.
>>          if (start_young_cycle()) {
>>            log_info...
>>          }
>>          break;
>>       }
>>       default: // fall through
>>    }
>>    // sleep a tad before next check; sleep duration inversely proportional to allocation rate
>>    regulator_sleep();   // My worry is that heuristics says wait for old increment and we sleep longer than that? May be that's not an issue?
>> }
>> ...
>> 
>> 
>> The rest of the checks etc. seem already to be done in the `start_...` methods from what I can tell.
>> 
>> I may have missed some subtlety though.
>
> When regulating young and old cycles (i.e., generational mode), the regulator overrides the global heuristics decision to start a global cycle. In other modes, whether or not to unload classes is decided _after_ a cycle is started. However, the generational mode can only unload classes in a _global_ cycle, so we use the oom in metaspace to _trigger_ a global cycle (even if the global heuristic would not otherwise trigger a cycle).
> 
> The other subtlety here is that old cycles are only started when a young cycle wants to run. Each old cycle is preceded by a young "bootstrap" cycle, so the young heuristic trigger is still satisfied. This was meant to avoid the situation where old cycles are triggered shortly after a young cycle - which runs an unproductive "bootstrap" cycle. This is why we check the young heuristic and then the old heuristic before starting an old cycle.
> 
> Your comment about starting a global cycle (to unload classes) even during an old cycle is interesting though. There's no specific reason why we couldn't interrupt and abandon the old cycle in this case, but I think that's a change for another PR.

I'm not too worried about the regulator's sleep method. The new flag introduces a delay if an old cycle is running, but the delay to start a young cycle because the regulator thread was sleeping is unchanged. Note that the feature can also be disabled by setting `ShenandoahMinimumOldMarkTimeMs` to zero.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/371#discussion_r1433415130