RFR: 8328220: GenShen: Move generational mode operational parameters out of ShenandoahHeap

[Y. Srinivas Ramakrishna]

On Thu, 14 Mar 2024 21:46:14 GMT, William Kemper <wkemper at openjdk.org> wrote:

> * Many fields previously defined in `ShenandoahHeap` have been moved to `ShenandoahOldGeneration`.
> * Generational specific serviceability has been moved to `ShenandoahGenerationalHeap`.
> * Methods for sizing the old generation have been moved to `ShenandoahGenerationalHeap`.
> * `ShenandoahGenerationalHeap::heap()` now asserts that the generational mode is active.

I left a few comments. Happy to take a second look in case the suggestions are used and it leads to changes substantive enough to warrant a re-review. Looks good otherwise. Thanks for this cleanup/restructure and for the very nice documentation added in places.

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

> 89:   // budget is constrained by availability of free memory.
> 90:   size_t old_evacuation_reserve = heap->old_generation()->get_evacuation_reserve();
> 91:   size_t old_evacuation_budget = (size_t) ((double) old_evacuation_reserve / ShenandoahOldEvacWaste);

const

src/hotspot/share/gc/shenandoah/heuristics/shenandoahYoungHeuristics.cpp line 82:

> 80:                                                             size_t cur_young_garbage) const {
> 81: 
> 82:   auto heap = ShenandoahGenerationalHeap::heap();

I have noticed that you "auto" in a few places but not in others (any reason why it's been done selectively? :-). Is there a more desirable end-state between the possible idiomatic alternatives?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahYoungHeuristics.cpp line 91:

> 89:   // This is young-gen collection or a mixed evacuation.
> 90:   // If this is mixed evacuation, the old-gen candidate regions have already been added.
> 91:   size_t max_cset = (size_t) (heap->young_generation()->get_evacuation_reserve() / ShenandoahEvacWaste);

const

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1185:

> 1183:   // Consult old-region surplus and deficit to make adjustments to current generation capacities and availability.
> 1184:   // The generation region transfers take place after we rebuild.
> 1185:   size_t old_region_surplus = old_generation->get_region_surplus();

To my earlier comment regarding carrying two variables one for +ve and one for -ve case, it looks like this would be text book case where a single variable with sign would shrink the code to half, make it more readable and get rid of the unnecessary if-then-else.

Not sure if it's also used somewhere else or how the values are set. May be worth a separate cleanup ticket.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 120:

> 118: }
> 119: 
> 120: ShenandoahGenerationalHeap::TransferResult ShenandoahGenerationalHeap::balance_generations() {

To a related comment on signed value of surplus (or deficit, as you will), one might probably change this if its only current role is to carry the directional information (by compressing all of that information into a sign, with canonical direction based on convention). You could still use this to make logging easier, though.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 144:

> 142:       success, old_region_deficit, "old"
> 143:     };
> 144:   }

Here again, a single field with sign and a sign comparison would work fine and may be even make it more natural.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 248:

> 246: 
> 247:   old_generation()->set_region_surplus(old_region_surplus);
> 248:   old_generation()->set_region_deficit(old_region_deficit);

We could just do a single signed quantity here. Not sure if the computation of the surplus/deficit could be simplified. Likely not.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 45:

> 43:   // be positive simultaneously.
> 44:   size_t _region_surplus;
> 45:   size_t _region_deficit;

This is an excellent comment and begs the question whether having an ssize_t to halve the representable range but carrying the surplus/deficit information as a sign would be better or worse for any reason. One variable is better than two is an argument in favor of one variable, but there may be reasons to need the greater range or problems with manipulating these where they are used?

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 52:

> 50: 
> 51:   // Bytes reserved within old-gen to hold the results of promotion. This is separate from
> 52:   // and in addition to the evacuation reserve for intra-generation evacuations.

Is there a name for "evacuation reserve for intra-generational evacuation in old"? If so, let's reference that field/variable here by its name?

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 102:

> 100: 
> 101:   // This is used to return unused memory from a retired promotion LAB
> 102:   size_t unexpend_promoted(size_t decrement);

May be mention in the `expend_promoted` comment that the accounting expends forward the space for an entire PLAB when its allocated, but "unexpends" any unused portion thereof when the PLAB is retired.

What this tells me is that `_promoted_expended` is non-monotonic in the evacuation phase as we expend and unexpend, which means that checks against promoted reserve might also be subject to "flickering" a bit during the latter phases of evacuation.

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/406#pullrequestreview-1944394753
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529453330
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529456625
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529457610
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529449570
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529469015
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529450374
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529452293
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529410676
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529413788
PR Review Comment: https://git.openjdk.org/shenandoah/pull/406#discussion_r1529442124