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

[earthling-amzn]

On Fri, 13 Nov 2020 17:11:56 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> 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.
>
> 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)",

Done.

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

The change was certainly originally motivated by having these changes in a separate heuristic. I don't think it hurts readability, but I'll inline them back in for the sake of performance.

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

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