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

Left a few more comments.

I'll make another pass over the free set dot cpp file once these comments have been addressed or otherwise resolved, probably in the next version/iteration of this PR.

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

> 922:     // We move all young available regions into mutator_free set and then we take back the regions we need for our
> 923:     // reserve.  This allows us to "compact" the collector_free (survivor) regions at the high end of the heap.
> 924:     clear_mutator_free(idx);

The `clear_mutator_free()` isn't consistent with the comment `move all young available regions into mutator_free set`.

May be you can change the documentation comment to state:

We examine each region in turn and assign it to the appropriate unique subset based on the following criteria:
... list of criteria used to assign to one or other of the subsets.

Such a comment might help clarify the intent of the code below. Also, if the set abstraction were used with assertions moving into the set modification methods, then this method's logic and flow would be much easier to see. There is a leakage of abstraction happening here for the assertions because of the naked use of low-level constructs into the higher level logic of the algorithm for manipulating the set membership of the regions.

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

> 951:   }
> 952: 
> 953:   // Evac reserve: reserve trailing space for evacuations

I'd move the individual region assignment loop above into its own work method, and the remainder of the work below that adjusts the reserves into its own work method, if appropriate? Not sure if such a refactor makes sense or not, but might make the code arguable more maintainable/readable?

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

> 974:     }
> 975:   }
> 976:   reserve_regions(young_reserve, old_reserve);

So I see now that this does a further iteration (below) over the regions until a specific size criteria is met, albeit iterating over the regions in the opposite order. While I have not understood the high level idea of the two loops, one thing to consider if whether the two iterations, one forward and one backward are needed, or if they could somehow be fused into a single iteration. Just tossing it out there as a question that you might be able to answer immediately, probably in the negative.

A brief couple of lines describing the idea of the two loops above and below would be useful.

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

PR Review: https://git.openjdk.org/shenandoah/pull/250#pullrequestreview-1384150258
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165988588
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165994376
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165997565