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

[Aleksey Shipilev]

On Fri, 13 Nov 2020 22:37:12 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 six additional commits since the last revision:
> 
>  - Inline calls to gc decision methods (vestige of an earlier design)
>  - Use os::elapsedTime to avoid type issues and to be consistent with other heuristics code
>  - Reuse instantaneous_rate method instead of duplicating code
>  - Rename variables to improve readability
>  - Make logging messages more consistent
>  - Restore call to reset allocation counter at cycle start

I did some performance tests on the variant of this patch, and while it regresses throughput in some scenarios, that's the price we pay for additional spike safety. I think we can drop current alloc-spike and penalty tracking (even from `shenandoahHeuristics`?).

Another round of tune-ups follows.

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

> 358:   double time_delta_sec = time - _last_sample_time;
> 359:   return allocation_delta / time_delta_sec;
> 360: }

GitHub review says there is no new-line at the end of file. Please add.

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

> 355: 
> 356: double ShenandoahAllocationRate::instantaneous_rate(double time, size_t allocated) const {
> 357:   size_t allocation_delta = allocated - _last_sample_value;

Please protect from accidental underflow and division by zero: 

 size_t last_value = _last_sample_value;
 size_t last_time = _last_sample_time;
 size_t allocation_delta = (allocated > last_value) ? (allocated - last_value) : 0;
 double time_delta = (time - last_time);
 return (time_delta > 0) ? (allocation_delta / time_delta) : 0;

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

> 303: 
> 304: ShenandoahAllocationRate::ShenandoahAllocationRate(ShenandoahAdaptiveHeuristics *heuristics) :
> 305:   _heuristics(heuristics),

Are we carrying `heuristics` here just to get access to `spike_threshold_sd`? I wonder if it is cleaner to just pass spike threshold to `is_spiking` then?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.hpp line 51:

> 49: 
> 50:   double instantaneous_rate(double time, size_t allocated) const;
> 51: 

Group the declarations like this:

public:
  explicit ShenandoahAllocationRate(ShenandoahAdaptiveHeuristics* heuristics);
  void allocation_counter_reset();

  void sample(size_t allocated);

  double instantaneous_rate(size_t allocated) const;
  double upper_bound(double standard_deviations) const;
  bool is_spiking(double rate) const;

 private:
  double instantaneous_rate(double time, size_t allocated) const;

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

> 324: }
> 325: 
> 326: double ShenandoahAllocationRate::upper_bound(double standard_deviations) const {

Name the parameter `sds`?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.hpp line 94:

> 92:   const static double HIGHEST_EXPECTED_AVAILABLE_AT_END;
> 93: 
> 94:   friend class ShenandoahAllocationRate;

Does it still need to be friends? I.e. are there non-public members in `ShenandoahAllocationRate` this class needs to access?

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

Changes requested by shade (Reviewer).

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