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

[Y. Srinivas Ramakrishna]

On Thu, 21 Dec 2023 00:29:19 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Young collections are allowed to interrupt old collections. Under heavy load, this may result in starvation of old collections - which may in turn lead to even more memory pressure on the young generation. Adding a minimum guaranteed amount of time for the old gc to make progress will help avoid this situation.
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update stale comment

A few comments for your consideration.

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()) {
           log_info...
        } else if (start_young_cycle()) {
           log_info...
        }
        break;
     }
     case SCT::servicing_old: {
         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.

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

> 180: }
> 181: 
> 182: bool ShenandoahRegulatorThread::should_unload_classes() {

I'd rename this to:

`should_start_metaspace_gc()`

to avoid confusion with similarly named method in other classes (heuristics) with a different semantics.

Relatedly, do we still need the second check in this method?


bool ShenandoahHeuristics::should_unload_classes() {
  if (!can_unload_classes()) return false;
  if (has_metaspace_oom()) return true;
  return ClassUnloadingWithConcurrentMark;
}


If not, it can be simplified to `return can_unload_classes() && ClassUnloadingWithConcurrentMark;`

I do believe, we should write a line documenting the intended semantics of `should_unload_classes()` in all these places because there ay otherwise be some confusion about what the truth modality "should" really means.

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

PR Review: https://git.openjdk.org/shenandoah/pull/371#pullrequestreview-1791966142
PR Review Comment: https://git.openjdk.org/shenandoah/pull/371#discussion_r1433342277
PR Review Comment: https://git.openjdk.org/shenandoah/pull/371#discussion_r1433344949