RFR: Fix budgeting assertion [v2]

[Kelvin Nilsen]

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

>> 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?

Thanks for these suggestions.  Adding a comment and "folding" those two addition operations into one.

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

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