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

[Y. Srinivas Ramakrishna]

On Tue, 13 Feb 2024 01:01:17 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 use a larger value of max_old_reserve than you have available even after using the transfer from young.

I guess I am not understanding enough of the subsequent bounds; I am just looking at the equivalence of the old code (before my change), the current code (after my previous change), and your proposed change which basically appears to say that we must augment whatever is availabe in old with whatever young is willing to transfer to old. That should happen irrespective of the what the combination of young_reserve and SOERP happens to be, not just special casing the extremal case that the previous fix handled. (Think about what happens in the usual cases where this value is left at the default: your proposed change would have no effect as far as I can see.)

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

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