RFR: Fix budgeting assertion [v2]

[Y. Srinivas Ramakrishna]

On Wed, 10 Aug 2022 17:02:28 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

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

Might be a good idea to describe at line 97, what `cur_young_garbage` will track as you process regions and build the collection set. Similarly, young_curset etc.

I see three separate outer if/else statements covering respectively the cases of generational/global, generational/not global, and non-generational cases. Would it read more easily and make for more easily maintained code to refactor into 3 separate cset builders called for each of the three cases, if necessary from the top level method (but even better if called from different contexts to specialize early hoisting the check on the type of collection up into the highest caller that has the information, rather than having all of them in one single, somewhat long and somewhat unwieldy method?

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

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