RFR: Add generations to freeset [v5]

[Y. Srinivas Ramakrishna]

On Wed, 12 Apr 2023 15:08:29 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> ShenandoahFreeSet has not yet been modified to deal efficiently with the combination of old-gen and young-gen collection set reserves.  This PR makes changes so that we can distinguish between collector_is_free, old_collector_is_free, and mutator_is_free.  Further, it endeavors to keep each set of free regions tightly packed, so the range of regions representing each set is small.
>> 
>> In its current form, this no longer fails existing regression tests (except for known problems that are being addressed independently)
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove debug instrumentation

I thought I'd leave the comments so far, and finish the rest later today.

At a high level, I'd treat the old, young, etc. as being mutually exclusive subsets of the set of all free regions. Then usage of bitmaps for the set membership becomes an implementation detail that need not be exposed everywhere via verbage such as "set_x_free" "clear_x_free", etc. but rather treating them as set addition and removal operations.

So, `set_x_free` becomes `add_to_x`, `clear_x_free` becomes `remove_from_x`. In genera, abstracting the mathematical set membership concept above the details of its implementation makes the client code easier to read.

I'll look through the remainder of the code and complete the remainder of the review later today.

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

> 79:   HeapWord* try_allocate_in(ShenandoahHeapRegion* region, ShenandoahAllocRequest& req, bool& in_new_region);
> 80:   HeapWord* allocate_with_affiliation(ShenandoahAffiliation affiliation, ShenandoahAllocRequest& req, bool& in_new_region);
> 81:   HeapWord* allocate_old_with_affiliation(ShenandoahAffiliation affiliation, ShenandoahAllocRequest& req, bool& in_new_region);

Given the ubiquity of names `foo()` and `old_foo()`, would it be correct to assume that the former is always `young`? Is there a reason not to use `young_foo()`? (I reask the question further below in a specific case, but it'd be great to understand the nomenclature here. It would be good to may be clarify in a documentation meta-comment somewhere, if not already done.

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

> 81:   HeapWord* allocate_old_with_affiliation(ShenandoahAffiliation affiliation, ShenandoahAllocRequest& req, bool& in_new_region);
> 82: 
> 83:   // While holding the heap lock, allocate memory for a single object which is to be entirely contained

I see that the callers of `allocate_...()` assert that the heap is locked, but would it be a good idea to also have these `allocate_...()` methods do so as well? May be I am being paranoid.

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

> 100:   bool touches_bounds(size_t num) const;
> 101:   bool expand_collector_bounds_maybe(size_t idx);
> 102:   bool expand_old_collector_bounds_maybe(size_t idx);

A 1-line documentation comment for each of these methods would be nice.

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

> 114:   ShenandoahFreeSet(ShenandoahHeap* heap, size_t max_regions);
> 115: 
> 116:   // Number of regions dedicated to GC allocations (for evacuation) that are currently free

In the generational case, are these the free (i.e. completely empty and not yet allocated) regions dedicated for use by only the young collector? If so, why not call it `young_collector_count()` consistent with `old_collector_count()` below?

Similarly for the underlying bitmap as well to `_young_collector_free_bitmap`?

Or is it the case that this is also used for the non-generational setting as well and hence the name without the `young` prefix, cf the `old` prefix below?

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 664:

> 662: }
> 663: 
> 664: 

Extra line probably unnecessary?

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

Changes requested by ysr (Author).

PR Review: https://git.openjdk.org/shenandoah/pull/250#pullrequestreview-1382379743
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1164856439
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1164898418
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1164849248
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1164846436
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1164823102