RFR: 8325670: GenShen: Allow old to expand at end of each GC

[Kelvin Nilsen]

On Mon, 19 Feb 2024 17:19:35 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>>> We special case ShenandoahOldEvacRatioPercent==100 because the "other case" has divide by (100 - ShenandoahOldEvacRatioPercent), which becomes divide by zero.
>> 
>> Yes, that I realize. I was asking about the addition of xfer_limit in just this case and not otherwise.
>> 
>>> 
>>> To generalize the form of the other expression, if ShenandoahOldEvacRatioPercent is 100, then there is no bound on maximum_old_evacuation_reserve. Or in other words, the bound is infinity times maximum_young_evacuation_reserve.
>> 
>> Correct. So I bounded it by max available. You corrected it to max_available + xfer_limit. It seems as if you want to bound everything by (max_available + xfer_limit).
>> 
>>> 
>>> In the original code, before the referenced change, if we can get past the divide-by-zero issue, we would find expansion of old to be limited by the xfer_limit at line 1265: if (old_region_deficit > max_old_region_xfer) { old_region_deficit = max_old_region_xfer; }
>>> 
>> 
>> That's still the case with old_region_deficit without your current change.
>> 
>>> We still ultimately limit expansion by xfer_limit.
>> 
>> I think that happened before as well, except when now because of your change we treat SOERP=100 specially (but nothing else).
>>> 
>>> I may have misunderstood your questions. Please let me know if I missed the mark.
>> 
>> What I am suggesting is that where we used to do:
>> 
>>   const size_t max_old_reserve = (ShenandoahOldEvacRatioPercent == 100) ?
>>      old_available : MIN2((young_reserve * ShenandoahOldEvacRatioPercent) / (100 - ShenandoahOldEvacRatioPercent),
>>                           old_available);
>> 
>> 
>> instead of doing what you suggest above, viz.:
>> 
>> 
>>   // In the case that ShenandoahOldEvacRatioPercent equals 100, max_old_reserve is limited only by xfer_limit.
>>   const size_t max_old_reserve = (ShenandoahOldEvacRatioPercent == 100) ?
>>     old_available + xfer_limit: (young_reserve * ShenandoahOldEvacRatioPercent) / (100 - ShenandoahOldEvacRatioPercent);
>> 
>> 
>> that we do:
>> 
>> 
>>   const size_t max_old_reserve = (ShenandoahOldEvacRatioPercent == 100) ?
>>      (old_available + xfer_limit) : MIN2((young_reserve * ShenandoahOldEvacRatioPercent) / (100 - ShenandoahOldEvacRatioPercent),
>>                                          old_available + xfer_limit);
>> 
>> 
>> Effectively, you are using `old_available + xfer_limit` for what we can ever have for the maximum size of old_reserve. Otherwise, for suitably large values of ShenandoahOldEvacRatioPercent, you'll us...
>
> With this PR, I was attempting to restore the "normal-case behavior" (when ShenandoahOldEvacRatioPercenet != 100) to how it behaved before https://github.com/openjdk/shenandoah/pull/369
> 
> Before that change, this line of code did not impose any restriction on the size of young_evacuation_reserve based on old_available:
> 
>  size_t maximum_old_evacuation_reserve =  maximum_young_evacuation_reserve * ShenandoahOldEvacRatioPercent / (100 - ShenandoahOldEvacRatioPercent);
> 
> For this new code, I invented an "artificial limit" to replace "infiinity" in the case that ShenandoahOldEvacRatioPercent equals 100.
> 
> Having studied this issue in the current implementaiton, I am inclined to pursue an even more aggressive change in a distinct [PR](https://github.com/openjdk/shenandoah/pull/395), which allows OLD to grow not only by stealing memory from the Mutator's excesses, but also by borrowing from the Young Collector's reserves.  So I'd prefer not to place more restrictions on the allowed growth of old at this line of code.

If you feel more comfortable, I can put the MIN2 expression into the normal case handling.  But I'll be wanting to take it out in the upcoming complementary PR.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/394#discussion_r1494864998