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

[earthling-amzn]

On Tue, 10 Nov 2020 17:30:40 GMT, Aleksey Shipilev <shade 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).
>
> 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?

You're right, the `log_info` macro performs this check before evaluating the arguments. I'll fix this.

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

Used on line 180 - I will inline the variable where it's 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?

`should_start_gc` is declared `const`, so it's cast away here. I didn't want to change the signature of `should_start_gc`, but if it were not `const` we could get rid of the cast here.

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

Refactored this to the pattern you suggested.

> 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();
> }

Done.

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

Yes, this is gone now.

> 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*`?

Yes, will fix this.

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

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