RFR: 8311843: GenShen: assertion failed "Old generation affiliated regions must be less than capacity" [v2]

[William Kemper]

On Fri, 25 Aug 2023 23:26:11 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1013:
>> 
>>> 1011:   _free_sets.move_to_set(idx, OldCollector, region_capacity);
>>> 1012:   _free_sets.assert_bounds();
>>> 1013: 
>> 
>> IIUC, the new code will fail to transfer if the transfer makes YOUNG smaller than min_capacity or makes OLD larger than max_capacity.  It will never fail because unaffiliated_regions < 1.
>> 
>> I have two concerns with the code as written:
>> 1. I don't think we should flip the region if we don't flip the capacity.  That will create future assertion failures.
>> 2. If we fail to flip_to_old(), we need to return a status code to so indicate.  The caller needs to know that the requested flip cannot be achieved and thus the precipitating "allocation request" must fail.
>> 
>> Alternatively, we could adopt the convention that min/max generation sizes are "preferences" but are not absolutely enforced.  Maybe it is better to bend the rules to allow pause-free operation rather than forcing an old evacuation failure that would require a Full GC.
>
> If we adopt this convention, we should get rid of any asserts that endeavor to enforce max/min generation capacities  (or change these to enforce that capacity never shrinks below 0 and never grows above heap->capacity())

`transfer_to_old` does check if young has enough affiliated regions to transfer:

  if (young_gen->free_unaffiliated_regions() < regions) {
    return false;


I think the original error here was caused by the Mutator free set holding many more regions than were available to the young generation (the issue with `reserve_regions`). I don't expect that to happen again, but I did add a log message if the transfer fails.

We are already somewhat loose when it comes to enforcing generational capacities. Are you proposing we have `shFreeSet` be the sole arbiter of capacity? That sounds like a good simplification - also sounds like a different PR?

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/311#discussion_r1306224321