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

[Aleksey Shipilev]

On Thu, 12 Nov 2020 22:57: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 two additional commits since the last revision:
> 
>  - Remove const qualifier from should_start_gc
>    
>    This lets the heuristics update state without const_casts.
>  - Rename ShenandoahReactive flags to ShenandoahAdaptive flags.

The more I study this patch, the more I like it, good job. Another round of changes below. I fixed a few mentioned issues in my private working copy while doing initial performance testing. Would be good to have an updated PR for testing too.

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

> 365:   _last_sample_time = os::javaTimeNanos();
> 366:   _last_sample_value = 0;
> 367: }

This method seems unused.

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

> 381: 
> 382: double ShenandoahAllocationRate::instantaneous_rate(size_t bytes_allocated_since_gc_start) const {
> 383:   size_t allocation_delta = bytes_allocated_since_gc_start - _last_sample_value;

`allocation_delta` underflows here in my benchmarks, causing back-to-back "Instantaneous rate" triggers. Note that `sample()` method seems to check for it.

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

> 291:   double instantaneous_rate = _allocation_rate.instantaneous_rate(bytes_allocated_since_gc_start);
> 292:   if (_allocation_rate.is_spiking(instantaneous_rate) && average_cycle_seconds > allocation_headroom / instantaneous_rate) {
> 293:     log_info(gc)("Trigger: Instantaneous allocation rate (%.0f %sB/s) will deplete free headroom (" SIZE_FORMAT "%s) before average time (%.2f ms) to complete GC cycle.",

If you compare the message with previous trigger, you'll probably notice has the different order of printed values: gc time, allocation rate, head room. This one should have the same order. You can change the previous trigger to read:

    log_info(gc)("Trigger: Average GC time (%.2f ms) is above the time for average allocation rate (%.0f %sB/s) to deplete free headroom (" SIZE_FORMAT "%s) (margin of error = %.2f)"

...and this one to read:

    log_info(gc)("Trigger: Average GC time (%.2f ms) is above the time for instantaneous allocation rate (%.0f %sB/s) to deplete free headroom (" SIZE_FORMAT "%s) (margin of error = %.2f)",

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

> 330: ShenandoahAllocationRate::ShenandoahAllocationRate(ShenandoahAdaptiveHeuristics *heuristics) :
> 331:   _heuristics(heuristics),
> 332:   _last_sample_time(os::javaTimeNanos()),

My IDE complains that `size_t` and `jlong` are not compatible here. That might be the problem for x86_32. Note that existing heuristics code uses `os::elapsedTime()`, which returns `double`. Is there a reason not to use it?

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

> 221:   if (is_allocation_rate_too_high(capacity, available, bytes_allocated_since_gc_start)) {
> 222:     return true;
> 223:   }

Does this split really win us anything readability-wise? I inlined the methods back, and the method is still quite readable, and does not require tracking `return`-s and pushing local variables there. I wonder if that is just a left-over from adaptive/reactive split.

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

> 264:   size_t allocation_headroom = available;
> 265: 
> 266:   size_t spike_headroom = capacity / 100 * ShenandoahAllocSpikeFactor;

I suspect `ShenandoahAllocSpikeFactor` handling is not needed anymore, as this new code is supposed to provide the similar cushion of few sigmas off the average value. That was my poor-mans solution to the same problem this patch resolves. You can remove the flag and update the comments in this block to mention only accumulated penalties.

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

> 128:   ShenandoahHeuristics::record_success_concurrent();
> 129: 
> 130:   double available = ShenandoahHeap::heap()->free_set()->available();

This `ShenandoahFreeSet::available()` returns `size_t`. If you keep it `size_t` as local variable, then you would not need to convert it back to `size_t` below. Plus, implicit conversions to `double` would happen in the computation in the next block.

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

> 343:       size_t allocation_delta = bytes_allocated_since_gc_start - _last_sample_value;
> 344:       size_t time_delta_ns = now - _last_sample_time;
> 345:       double alloc_bytes_per_second = ((double) allocation_delta * NANOUNITS) / time_delta_ns;

I wonder if you can just call `instantaneous_rate()` here. Note the comment about that method too.

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

> 205:   available = (available > soft_tail) ? (available - soft_tail) : 0;
> 206: 
> 207:   size_t bytes_allocated_since_gc_start = heap->bytes_allocated_since_gc_start();

Name this local variable `allocated`, it unclutters some code.

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

> 270:   allocation_headroom -= MIN2(allocation_headroom, penalties);
> 271: 
> 272:   double average_cycle_seconds = _gc_time_history->davg() + (_margin_of_error_sd * _gc_time_history->dsd());

Name this local variable `avg_cycle_time`, makes it easier to read.

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

> 271: 
> 272:   double average_cycle_seconds = _gc_time_history->davg() + (_margin_of_error_sd * _gc_time_history->dsd());
> 273:   double bytes_allocated_per_second = _allocation_rate.upper_bound(_margin_of_error_sd);

Name this local variable `avg_alloc_rate`.

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

> 289:   }
> 290: 
> 291:   double instantaneous_rate = _allocation_rate.instantaneous_rate(bytes_allocated_since_gc_start);

Name this variable `instant_alloc_rate`.

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

> 268: 
> 269:   allocation_headroom -= MIN2(allocation_headroom, spike_headroom);
> 270:   allocation_headroom -= MIN2(allocation_headroom, penalties);

...in fact, maybe even penalties are not needed since we are widening the SD "windows" on degenerated/full GC? We can clean this up later if we are not sure.

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

> 367: }
> 368: 
> 369: bool ShenandoahAllocationRate::is_spiking(double instantaneous_rate) const {

Name the argument just `rate`?

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

> 368: 
> 369: bool ShenandoahAllocationRate::is_spiking(double instantaneous_rate) const {
> 370:   double standard_deviation = _rate.sd();

Name it `sd`, that is pretty conventional.

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

> 384:   size_t time_delta_ns = os::javaTimeNanos() - _last_sample_time;
> 385:   double alloc_bytes_per_second = ((double) allocation_delta * NANOUNITS) / time_delta_ns;
> 386:   return alloc_bytes_per_second;

Inline local variable here.

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

> 380: }
> 381: 
> 382: double ShenandoahAllocationRate::instantaneous_rate(size_t bytes_allocated_since_gc_start) const {

Name the argument just `allocated`?

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

> 337: }
> 338: 
> 339: void ShenandoahAllocationRate::sample(size_t bytes_allocated_since_gc_start) {

Name the argument just `allocated`?

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

Changes requested by shade (Reviewer).

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