RFR: Add generations to freeset [v20]

[Y. Srinivas Ramakrishna]

On Fri, 28 Apr 2023 15:36:23 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:
> 
>   Fix white space

Approach looks good; I left a few initial comments, but will work through the details later.

In my opinion I think the whole thing can be in the same PR, no need to break it up (to your question above).

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

> 30: #include "gc/shenandoah/shenandoahHeap.hpp"
> 31: 
> 32: enum MemoryReserve : uint8_t {

Would it tidy up matters to move `ShenandoahSetsOfFree` into its own source file(s)? I realize it's only used by `ShenandoahFreeSet`, but I think it'll keep it all much cleaner.

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

> 30: #include "gc/shenandoah/shenandoahHeap.hpp"
> 31: 
> 32: enum MemoryReserve : uint8_t {

`FreeMemoryType` or `MemoryType` instead of `MemoryReserve` for the enum type name ?

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

> 39: 
> 40: class ShenandoahSetsOfFree {
> 41:   friend class ShenandoahFreeSet;

Is the friendliness necessary, or is this a case of leakage of abstraction. Could the requisite stuff just be made public? May be as I look more closely at uses of non-public methods by the friend, I'll understand why the friendliness seems necessary. It'd be good to document that, if nothing else. That process of documentation itself might make one see a better approach.

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

> 42: 
> 43: private:
> 44:   size_t _max;

What does `_max` hold?

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

> 85:   inline bool in_free_set(size_t idx, MemoryReserve which_set) const;
> 86: 
> 87:   // Each of the following four methods returns _max to indicate absence of requested region.

Are these expected invariants for returned values for queries on a static set `which_set`:

-  `0 <= leftmost <= leftmost_empty <= rightmost_empty <= rightmost <= _max - 1`, whenever `which_set` is not empty
- `_max`, whenever `which_set` is empty

?

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

PR Review: https://git.openjdk.org/shenandoah/pull/250#pullrequestreview-1406733198
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1180893488
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1180896661
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1180894534
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1180889756
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1180889136