RFR: 8311602 GenShen: Decouple generational mode heuristics [v2]

[Kelvin Nilsen]

On Mon, 10 Jul 2023 17:42:19 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> The general idea here is to straighten out code paths which fork based on the mode configuration. A `ShenandoahYoungHeuristics` class has been added to host the logic specific to the generational mode. In many cases, the entangled generational code can simply be moved behind methods which override existing virtual methods. 
>> 
>> A few other notable changes:
>> * The confusing "trigger" heuristic has been removed from `ShenandoahOldHeuristics`.
>>   * The old heuristic defines its own triggers now. It is no longer possible to specify the old generation's trigger heuristic.
>> * Selection of aged regions for in-place promotion has been moved to `ShenandoahGeneration`.
>>   * The method implementation does not depend on any members of `ShenandoahHeuristics`. Moving the method therefore removes an unnecessary dependency on the heuristics.
>> * A new `ShenandoahHeapCharacteristics` interface has been added to provide information about the heap _or generation_ to the heuristics.
>>   * On the development branch, this interface is implemented by `ShenandoahGeneration`.
>>   * The plan is to upstream this interface and have it implemented by `ShenandoahHeap` 
>> 
>> The taxonomy of these heuristic classes has become somewhat more complex - here is a chart showing the relationships:
>> 
>> 
>> ShenandoahHeuristics
>> -  ShenandoahPassiveHeuristics
>> -  ShenandoahCompactHeuristics
>> -  ShenandoahAggressiveHeuristics
>> -  ShenandoahStaticHeuristics
>> -  ShenandoahOldHeuristics
>> +  ShenandoahAdaptiveHeuristics
>>   +  ShenandoahGenerationalHeuristics
>>     -  ShenandoahYoungHeuristics
>>     -  ShenandoahGlobalHeuristics
>
> William Kemper has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Remove overzealous assert
>  - Fix missing imports (windows build failure)

Thank you very much for working through all these details.  Looks good.  I assume we've compared performance pipeline results and see no regresions.

Looks like the Windows x86 tier1 test failures needs to be resolved still.

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

> 56: const double ShenandoahAdaptiveHeuristics::MAXIMUM_CONFIDENCE = 3.291; // 99.9%
> 57: 
> 58: ShenandoahAdaptiveHeuristics::ShenandoahAdaptiveHeuristics(ShenandoahHeapCharacteristics* heap_info) :

I'm assuming we'll issue a separate mainline patch that does slight mainline refactoring so we'll have less diffs when we integrate GenShen upstream.   I wonder if it would be easier to review this if we saw that refactoring first?

This gets us much closer to the original code, but still has the new heap_info argument to this constructor.

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

> 128: }
> 129: 
> 130: void ShenandoahAdaptiveHeuristics::record_success_concurrent(bool abbreviated) {

Note to self: the abbreviated argument was not present in original single-generation shenandoah code, but this can be introduced with a mainline refactoring effort.

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

> 329: }
> 330: 
> 331: // Return a conservative estimate of how much memory can be allocated before we need to start GC. The estimate is based

Note to self: this code is moved to shenandoahYoungHeuristics, which is a subclass of ShenandoahGenerationalHeuristics, which is subclass of ShenandoahAdaptiveHeuristics.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 544:

> 542: 
> 543:   // Otherwise, defer to inherited heuristic for gc trigger.
> 544:   return ShenandoahHeuristics::should_start_gc();

I'm puzzling over this.  May resolve for myself after reading more code.  Not clear to me how ShenandoahHeuristics knows that I'm asking about trigger for OLD vs YOUNG.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp line 80:

> 78:   // Flag is set when promotion failure is detected (by gc thread), and cleared when
> 79:   // old generation collection begins (by control thread).
> 80:   // TODO: This is set, but never read.

I vote to remove this.  In current implementation, failure to promote is considered a normal happening, and does not necessarily trigger the start of an old-marking pass, nor expansion of OLD.

src/hotspot/share/gc/shenandoah/mode/shenandoahPassiveMode.cpp line 29:

> 27: #include "gc/shenandoah/heuristics/shenandoahPassiveHeuristics.hpp"
> 28: #include "gc/shenandoah/mode/shenandoahPassiveMode.hpp"
> 29: #include "gc/shenandoah/shenandoahGeneration.hpp"

Are we sure we need this include?  I see no other changes in this source file so it looks suspicious.

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

Marked as reviewed by kdnilsen (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/292#pullrequestreview-1522795411
PR Comment: https://git.openjdk.org/shenandoah/pull/292#issuecomment-1629753805
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1258883671
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1258889649
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1258894170
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1258792912
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1258753755
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1258920536