RFR: 8255984: Shenandoah: "adaptive" heuristic is prone to missing load spikes

[Aleksey Shipilev]

On Fri, 6 Nov 2020 20:00:25 GMT, earthling-amzn <github.com+71722661+earthling-amzn at openjdk.org> wrote:

> This change adds a "reactive" heuristic for triggering concurrent GC cycles.
> 
> The reactive heuristic maintains a margin of error and an allocation spike detection mechanism to trigger cycles somewhat more aggressively than the 'adaptive' heuristic. This heuristic 'reacts' to the outcome of GC cycles by adjusting the sensitivity of the triggers.
> 
> JBS ticket is here: https://bugs.openjdk.java.net/browse/JDK-8255984
> 
> The "adaptive" heuristic remains the default.
> 
> Steps to reproduce and test will follow shortly (there are no new jtreg test failures for Shenandoah with this change).

While waiting for merge to "adaptive", here is a round of minor things from the first read.

Thanks for updates. I would like to dive in with some testing. Second read nits below, meanwhile.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 135:

> 133:   if (available < min_threshold) {
> 134:     if (log_is_enabled(Info, gc)) {
> 135:       log_info(gc)("Trigger: Free (" SIZE_FORMAT "%s) is below minimum threshold (" SIZE_FORMAT "%s)",

Why do we need these additional checks? Is this to avoid computing the arguments?  I was under impression `log_info` and friends do this right already. Are they?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 167:

> 165:   //   1. Some space to absorb allocation spikes
> 166:   //   2. Accumulated penalties from Degenerated and Full GC
> 167:   ShenandoahHeap* heap = ShenandoahHeap::heap();

Does not seem to be used.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahReactiveHeuristics.cpp line 78:

> 76: 
> 77:   // Track allocation rate even if we decide to start a cycle for other reasons.
> 78:   ShenandoahReactiveHeuristics *heuristic = const_cast<ShenandoahReactiveHeuristics *>(this);

Why this cast is needed? Aren't local fields accessible directly, even without `heuristics->`, or `this->`, for that matter?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahReactiveHeuristics.cpp line 219:

> 217: }
> 218: 
> 219: static double safe_adjust(double value, double adjustment, double min, double max) {

Seems like this is just:

static double saturate(double value, double min, double max) {
  return MAX2(MIN2(value, max), min);
}

...in fact, we probably use this pattern without the explicit method everywhere else.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahReactiveHeuristics.cpp line 236:

> 234:   } else if (_last_trigger == SPIKE) {
> 235:     adjust_spike_threshold(amount);
> 236:   }

Please use this style:

switch(_last_trigger) {
  case RATE:
    adjust_margin_of_error(amount);
    break;
  case SPIKE:
    adjust_spike_threshold(amount);
    break;
  case OTHER:
    // Do nothing
    break;
  default:
    ShouldNotReachHere();
}

src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 158:

> 156:           range(0,1.0)                                                      \
> 157:                                                                             \
> 158:                                                                             \

Double new-line.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahReactiveHeuristics.cpp line 96:

> 94:   // Don't call into our immediate parent class, we've already done
> 95:   // everything it would do (and more).
> 96:   return ShenandoahHeuristics::should_start_gc(); // NOLINT(bugprone-parent-virtual-call)

We don't do lint (disable) warnings in the source code. I understand it might be useful for bugprone, but we don't do that anyway. This should go away once changes are merged with "adaptive", AFAICS.

src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 130:

> 128:           range(0,100)                                                      \
> 129:                                                                             \
> 130:   product(uintx, ShenandoahReactiveSampleFrequencyHz, 10, EXPERIMENTAL,     \

Minor nit: these are now `ShenandoahAdaptive*`?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 212:

> 210:   ShenandoahAdaptiveHeuristics *heuristic = const_cast<ShenandoahAdaptiveHeuristics *>(this);
> 211:   heuristic->_allocation_rate.sample(bytes_allocated_since_gc_start);
> 212:   heuristic->_last_trigger = OTHER;

Shouldn't `_last_trigger` updates be near `return true;` in this method? I.e. we should not probably overwrite `_last_trigger` if nothing was actually triggered? This probably affects the super-call to `ShenandoahHeuristics::should_start_gc()` as well?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 331:

> 329: }
> 330: 
> 331: 

Double newline.

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

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1099