RFR: 8327000: GenShen: Integrate updated Shenandoah implementation of FreeSet into GenShen [v8]

[William Kemper]

On Thu, 13 Jun 2024 22:22:01 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> The mainline implementation of ShenandoahFreeSet was recently updated.  This PR integrates the upstream changes
>> into Generational Shenandoah.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Minor refinements to test programs
>   
>   TestAllocIntArrays: comments to explain behavior.
>   TestOldGrowthTriggers: reduce the number of loop iterations so this test
>   will not time out on less powerful test platforms.

It looks like the comments for `ShenandoahFreeSet::internal_fragmentation` and `ShenandoahFreeSet::external_fragmentation` have been moved from the header into the implementation file when comparing against upstream (`jdk:master`). Can we keep them in-sync with upstream for the sake of minimizing differences here?

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

> 182:   _rightmosts_empty[int(ShenandoahFreeSetPartitionId::Mutator)] = mutator_rightmost_empty;
> 183: 
> 184:   _region_counts[int(ShenandoahFreeSetPartitionId::Mutator)] = mutator_region_count;

This statement duplicates statement on line 178.

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

> 206:   _rightmosts_empty[int(ShenandoahFreeSetPartitionId::OldCollector)] = old_collector_rightmost_empty;
> 207: 
> 208:   _region_counts[int(ShenandoahFreeSetPartitionId::OldCollector)] = old_collector_region_count;

Same duplication from `establish_mutator_intervals`.

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

> 342:   // At start of update refs: Collector => Mutator
> 343:   //                          OldCollector Empty => Mutator
> 344:   assert (((available <= _region_size_bytes) &&

This is quite hard for my feeble brain to parse. Could we break this into multiple asserts? or break some of the expressions out into functions?

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

> 973:     assert(!r->is_affiliated(), "New region " SIZE_FORMAT " should be unaffiliated", r->index());
> 974:     r->set_affiliation(req.affiliation());
> 975:     ShenandoahMarkingContext* const ctx = _heap->complete_marking_context();

Can we move this `ctx` closer to the `asserts` that use it, or perhaps inline the variable in the asserts?

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

> 1176: 
> 1177:   size_t remainder = words_size & ShenandoahHeapRegion::region_size_words_mask();
> 1178:   ShenandoahMarkingContext* const ctx = _heap->complete_marking_context();

`ctx` looks unused.

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

> 1224: }
> 1225: 
> 1226: void ShenandoahFreeSet::try_recycle_trashed(ShenandoahHeapRegion *r, bool is_generational) {

`is_generational` looks unused.

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

> 2064:            "Boundaries or find_first_set_bit failed: " SSIZE_FORMAT, index);
> 2065:     ShenandoahHeapRegion* r = _heap->get_region(index);
> 2066:     if (r->is_empty()) {

This algorithm has changed from upstream. Is that intentional?

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

Changes requested by wkemper (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/440#pullrequestreview-2123889333
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1643422326
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1643423178
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1643434756
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1643436394
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1643438862
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1643441101
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1643450046