RFR: Fix budgeting assertion [v2]

[Y. Srinivas Ramakrishna]

On Tue, 9 Aug 2022 16:19:39 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> An assertion failure revealed problems with evacuation budgeting computations.  Three changes are reflected in this pull request:
>> 
>> 1. Memory set aside for the promotion reserve was not excluded from the allocation supplement
>> 2. When the old evacuation reserve is larger than the memory consumed by old evacuation collection set, we loan the excess memory to allocation supplement to make sure this memory is not consumed by promotions.  The number of regions set aside from this reserve for the allocation supplement must be rounded down from the total excess memory.  (Before this patch, it was rounded up.)
>> 3. The assertion test that was failing needed a <= comparison instead of a < comparison.
>
> Kelvin Nilsen has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Remove instrumentation
>  - Fix choose_collection_set to use region->index after quicksort
>    
>    After region data is sorted in garbage order, index within the region
>    data array no longer matches the region->index().  Use the
>    region->index() to test whether the region was pre-selected rather than
>    using the index within the region data array.  Very confusing that this
>    code ever worked.  Perhaps this bug was introduced by some sort of merge
>    conflict.
>  - Assure that aged young regions are not promoted if not pre-selected
>    
>    In the case that an aged young region is not pre-selected, it should not
>    be included in the collection set because there is not enough room in
>    old-gen to hold its promoted contents.

Some minor comments on structure of the method and a possible refactoring, and on naming & documenting of some variables.

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

> 124:             old_cur_cset = new_cset;
> 125:           }
> 126:         } else if (cset->is_preselected(r->index())) {

It might be a good idea for all the `choose_collection_set*` methods that use `is_preselected` to assert that cset's `_preselected_regions` isn't null, perhaps via an `is_preselected_established` (or appropriately named assert_only method that asserts that `_preselected_regions != NULL`

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

> 134:           // reclaim highly utilized young-gen regions just for the sake of finding min_garbage to reclaim
> 135:           // within youn-gen memory
> 136:           cur_young_garbage += r->get_live_data_bytes();

Isn't `r->garbage() + r->get_live_data_bytes()` equal to `r->used()`?

I am not sure I understand the reasoning here. I would much rather we named the `cur_young_garbage` variable more correctly if it is counting the amount of young regions that are in some sense being "removed from the young generation" (if indeed that's how to interpret it).

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

PR: https://git.openjdk.org/shenandoah/pull/156