RFR: 8348400: GenShen: assert(ShenandoahHeap::heap()->is_full_gc_in_progress() || (used_regions_size() <= _max_capacity)) failed: Cannot use more than capacity # [v2]

[Y. Srinivas Ramakrishna]

On Wed, 12 Mar 2025 23:17:44 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Shenandoah cannot recycle immediate trash regions during the concurrent weak roots phase, however some of these regions may be assigned to the old generation collector's reserve. When an evacuation/promotion tries to allocate in such a region, it will fail (as expected) and try to 'steal' a region from the mutator's partition of the free set.  There are cases when this cannot be allowed due to capacity constraints. However, in some of these cases it will be possible to 'swap' a region between the old reserve and the mutator's partition. This change covers this case.
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert "Do not enforce size constraints on generations"
>   
>   This reverts commit 11ff0677449fa6749df8830f4a03f1c7861ba314.

Generally looks right, but a few comments for your consideration.

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

> 1291: 
> 1292:   ShenandoahGenerationalHeap* gen_heap = ShenandoahGenerationalHeap::heap();
> 1293:   const size_t region_capacity = alloc_capacity(r);

A general note on terminology. We have generally used "capacity" to mean the total space, including that which has been allocated, and "used" for the space that has been allocated and isn't available to allocate. I'd use "free" here and avoid the extra arithmetic.

I notice that the method actually uses "used", rather than "free".

I think the interface for _partitions `move_from_...` is unnecessarily fat. Since we send the region idx to the `move_from_...` method, why not let that method get the amount free, rather than passing it as an additional parameter?

I see that we essentially use this value only at line 1300 to correct the evacuation reserve figure. (Side question: Why don't we do that when we do the swap after line 1327?)

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

> 1319: 
> 1320:     if (unusable_trash != -1) {
> 1321:       // 2. Move it to the mutator partition

// Move the unusable trash region we found to the mutator partition.

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

> 1322:       _partitions.move_from_partition_to_partition(unusable_trash,
> 1323:                                                    ShenandoahFreeSetPartitionId::OldCollector,
> 1324:                                                    ShenandoahFreeSetPartitionId::Mutator, region_capacity);

Shouldn't `region_capacity` argument be the free space in the unusable trash region? Wouldn't that be 0 (else why "unusable"?)

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 327:

> 325:   // hold evacuated objects.  If this occurs and memory is still available in the Mutator's free set, we will flip a region from
> 326:   // the Mutator free set into the Collector or OldCollector free set.
> 327:   void flip_to_gc(ShenandoahHeapRegion* r);

It seems as if (the current implementation of) `flip_to_gc()` always succeeds in flipping. I'd add that to its spec comment.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 329:

> 327:   void flip_to_gc(ShenandoahHeapRegion* r);
> 328: 
> 329:   bool flip_to_old_gc(ShenandoahHeapRegion* r);

// Return true if and only if successfully flipped to old partition.

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

PR Review: https://git.openjdk.org/jdk/pull/23998#pullrequestreview-2714934231
PR Review Comment: https://git.openjdk.org/jdk/pull/23998#discussion_r2012841573
PR Review Comment: https://git.openjdk.org/jdk/pull/23998#discussion_r2012830401
PR Review Comment: https://git.openjdk.org/jdk/pull/23998#discussion_r2012835985
PR Review Comment: https://git.openjdk.org/jdk/pull/23998#discussion_r2012789624
PR Review Comment: https://git.openjdk.org/jdk/pull/23998#discussion_r2012790543