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

[Aleksey Shipilev]

On Tue, 10 Nov 2020 20:10:29 GMT, earthling-amzn <github.com+71722661+earthling-amzn at openjdk.org> wrote:

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

Right. Let's drop the `const` from declarations then!

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

Right. Ignore this comment then.

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

Good! (please click "Resolve conversation")

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

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