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

[Aleksey Shipilev]

On Fri, 20 Nov 2020 23:30:23 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).
>
> earthling-amzn has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Do not re-sample allocation rate with higher frequency
>    
>    This causes too many false positives and triggers unnecessary cycles.
>  - Sample allocation rate even if cumulative bytes allocated hasn't changed
>    
>    Without this, the average allocation rate will be much higher than it should be.

Okay, I have a minor suggestion that seems still preserve the semantics. It is generally looks good, the testing passes, and benchmarks look okay (some regressions, but expected). Feel free to integrate.

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

> 259:   }
> 260: 
> 261:   if (rate > 0.0) {

I'd prefer to do this in `is_spiking` instead, because it seems to conceptually relate to the definition of a spike. E.g.:

bool ShenandoahAllocationRate::is_spiking(double rate, double threshold) const {
  if (rate <= 0) { 
    return false;
  }
  ...

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

Marked as reviewed by shade (Reviewer).

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