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

[Kelvin Nilsen]

On Fri, 25 Aug 2023 23:31:21 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> There are a few fixes here:
>> 
>> During a Full GC, the compaction process is not constrained by the maximum capacity for young and old generations. We must, therefore, reestablish these constraints after the Full GC completes by _forcing_ the transfer of regions to old, if necessary.
>> 
>> When a region is "flipped" from the Mutator's free set to the Collector's, we cannot _force_ the transfer of the region because the young generation may not have any free regions.
>> 
>> A logic error in the loop which rebuilds the free set could prevent transferring free regions from the mutator to the collector reserves.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'shenandoah/master' into full-gc-max-capacity
>  - Do not prematurely exit when reserving regions
>  - 8314610: hotspot can't compile with the latest of gtest because of <iomanip>
>    
>    Reviewed-by: jiefu, stuefe
>  - Force transfer of old regions to old generation capacity after full GC
>  - Remove vestigial member and methods

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.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1228:

> 1226:     }
> 1227:   }
> 1228:   if (old_reserve > _free_sets.capacity_of(OldCollector)) {

Good catch here.  Sorry for the code as originally written.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 1539:

> 1537:         size_t old_regions_deficit = (old_usage - old_capacity) / ShenandoahHeapRegion::region_size_bytes();
> 1538:         heap->generation_sizer()->force_transfer_to_old(old_regions_deficit);
> 1539:       }

So here, we may end up violating max/min sizes of young generation.  I'm inclined to allow this if that is required to make the just-completed full gc valid.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 982:

> 980:   ShenandoahHeap::heap()->shenandoah_policy()->record_success_degenerated();
> 981: }
> 982: 

I gather that this code was redundant/not needed.  I'm not sure removal of this code was highlighted in the commit comment.  Maybe we should also mention some code cleanup regarding generation collection times.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/311#discussion_r1306206629
PR Review Comment: https://git.openjdk.org/shenandoah/pull/311#discussion_r1306210708
PR Review Comment: https://git.openjdk.org/shenandoah/pull/311#discussion_r1306214404
PR Review Comment: https://git.openjdk.org/shenandoah/pull/311#discussion_r1306215596