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

[Y. Srinivas Ramakrishna]

On Thu, 13 Jul 2023 21:05:23 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 one additional commit since the last revision:
> 
>   Record cycle end for old generation heuristic

Thanks for the code walk through which allowed me to understand the scope and objective of the changes much better. This generally looks good to me, and greatly cleans up the code changes that previously stemmed from the "new" generational mode.

- I left a few questions/comments in-line in a few places that caught my eye or where things were not clear to me.
- Some of this will be in files/classes that are in upstream, which we'd like not to change, but I'd really like to see us take a stab at writing a 1 or 2-sentence header comment for each of the derived classes or `ShenandoahHeuristics`. Hopefully those additions can be upstreamed in your other PR (or will not raise issues when the GenShen PR is put up for review as the documentation comments are easy to see as having not made any code changes). It tends to make code reading much more comprehensible by someone looking at the code for the first time or after a long gap. :-) I've left a comment to similar effect specifically for the class `ShenandoahOldHeuristic`.

Rest looks good to me. Thanks for doing this refactoring & clean-up!

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

> 137:   ShenandoahHeuristics::record_success_concurrent(abbreviated);
> 138: 
> 139:   size_t available = MIN2(_heap_stats->available(), ShenandoahHeap::heap()->free_set()->available());

I am a little bit confused here.

I'd expect this to be gone with the changes in the upstream jdk PR: https://github.com/openjdk/jdk/pull/14856/files#diff-5ca0a05384b7b2604dd3c9b55d91a7010a42cd0ba246600c844e19821bd6b60b and to read simply:


size_t available = _heap_stats->available();


Can you help me understand why that is not the case?

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

> 1: /*

I'd like to see a brief class documentation comment for each of the classes in the sub-class closure of `ShenandoahHeuristic` to sharpen the precise role & specialization that each of them offers (referring to its parent class). In particular, for the ShenandoahOldHeuristic, I'd also like to see a brief 1-sentence comment stating why it makes sense for it to derive directly from ShenandoahHeuristics, rather than from ShenandoahGenerationalHeuristic which the lay-reader might naively expect.

Thanks!

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 442:

> 440:         // Signal that promotion failed. Will evacuate this old object somewhere in young gen.
> 441:         report_promotion_failure(thread, size);
> 442:         handle_promotion_failure();

Why is this gone? Is it now handled somewhere else?

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/292#pullrequestreview-1525352534
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1263148563
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1263157218
PR Review Comment: https://git.openjdk.org/shenandoah/pull/292#discussion_r1260450067