RFR: Add generations to freeset [v5]

[Aleksey Shipilev]

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

Cursory review follows

src/hotspot/share/gc/shenandoah/mode/shenandoahGenerationalMode.hpp line 33:

> 31: public:
> 32:   virtual void initialize_flags() const;
> 33:   virtual ShenandoahHeuristics* initialize_heuristics(ShenandoahGeneration* generation) const override;

This reverts https://github.com/openjdk/shenandoah/commit/ab9a39713fafc2257cc34531fe638e82b7221067, should not be here.

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

> 63:   assert (idx < _max, "index is sane: " SIZE_FORMAT " < " SIZE_FORMAT " (left: " SIZE_FORMAT ", right: " SIZE_FORMAT ")",
> 64:           idx, _max, _mutator_leftmost, _mutator_rightmost);
> 65:   assert(!_mutator_free_bitmap.at(idx) || (alloc_capacity(ShenandoahHeap::heap()->get_region(idx)) > 0),

Things like `(alloc_capacity(ShenandoahHeap::heap()->get_region(idx)) > 0)` are used often here, might just use a separate method for it.

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

> 136:         ShenandoahHeapRegion* r = _heap->get_region(c);
> 137:         assert(r->is_trash() || (r->affiliation() == ShenandoahAffiliation::FREE)
> 138:                || (r->affiliation() == ShenandoahAffiliation::OLD_GENERATION),

Now replaceable with `!r->is_affiliated()`, `r->old()`, etc.

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

> 277:         if (allow_new_region) {
> 278:           // Then try a free region that is dedicated to GC allocations.
> 279:           if (req.affiliation() == ShenandoahAffiliation::OLD_GENERATION) {

Here and later, now replaceable with `req.is_old()`.

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

> 557:         assert(!is_mutator_free(idx) && !is_old_collector_free(idx), "Region cannot be in multiple free sets");
> 558:         adjust_collector_bounds_if_touched(idx);
> 559:       }

Do we ever get to generic `else` branch here? If not, we should assert that we are not expected to be there.

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

> 646:       }
> 647:     }
> 648:     _old_collector_search_left_to_right = (old_collector_available_in_second_half > old_collector_available_in_first_half);

So, if there are more regions available in the "left" (first) half, then we search from *right* to left? Is this the intent? We need to drop a comment here why.

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

> 650: }
> 651: 
> 652: bool ShenandoahFreeSet::expand_collector_bounds_maybe(size_t idx) {

For this and the following method, the return value is not used, can be dropped.

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

> 1063:         buffer[idx] = 'C';
> 1064:       }
> 1065:       else if (r->is_humongous()) {

Suggestion:

      } else if (r->is_humongous()) {

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 323:

> 321: 
> 322:     // The evacuation reserves for old-gen and young-gen are available
> 323:     VALID_EVACUATION_RESERVE_QUANTITIES_BITPOS = 6

I think we only do these when we want to access the flags from generated code. If we only do the accesses from the GC code, this could be just a plan `bool` field.

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

PR Review: https://git.openjdk.org/shenandoah/pull/250#pullrequestreview-1381912054
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1164518396
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165364256
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165368117
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165369385
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165373878
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165378828
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165382402
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165386365
PR Review Comment: https://git.openjdk.org/shenandoah/pull/250#discussion_r1165391275