RFR: 8342214: GenShen: Reduce code duplication in shFreeSet with iterator abstraction

[Y. Srinivas Ramakrishna]

On Mon, 14 Oct 2024 22:07:41 GMT, William Kemper <wkemper at openjdk.org> wrote:

> Introduce `ShenandoahLeftRightIterator` and `ShenandoahRightLeftIterator` classes to encapsulate allocation bias to one side of the heap or the other.

LGTM. A few minor comments which, if they should entail changes, don't require a re-review.

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

> 56:   idx_t _end;
> 57:   ShenandoahRegionPartitions* _partitions;
> 58:   ShenandoahFreeSetPartitionId _partition;

I'd call this a `_pid`, short for free set partition id. The type mismatch of `_partitions` (plural) and `_partition` (singular) makes the current form of naming a bit awkward, and the single-letter difference rife for confusion when reading code (although the type-system saves one here).

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

> 59: public:
> 60:   explicit ShenandoahLeftRightIterator(ShenandoahRegionPartitions* partitions, ShenandoahFreeSetPartitionId partition, bool use_empty = false)
> 61:     : _idx(0), _end(0), _partitions(partitions), _partition(partition) {

Here and in the right-biased iterator, why do we first do the 0-initialization, which will soon be over-written with the correct value. Why not just do the correct initialization once? That would allow you to make, e.g. `_end` a const. I'd similarly const all other fields other than `_idx` which is manipulated by the iterator.

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

> 749: }
> 750: 
> 751: HeapWord* ShenandoahFreeSet::allocate_from_partition_with_affiliation(ShenandoahAffiliation affiliation,

Since the "from_partition" part is decided in the method, based on the request, I'd rename this `allocate_with_affiliation(...)`.

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

> 294:   // Return the address of memory allocated, setting in_new_region to true iff the allocation is taken
> 295:   // from a region that was previously empty.  Return nullptr if memory could not be allocated.
> 296:   inline HeapWord* allocate_from_partition_with_affiliation(ShenandoahAffiliation affiliation,

The "from_partition" in the name of the method sounds a bit mysterious since the first argument of the old method is now gone. How about just `allocate_with_affiliation`, and let the method signature distinguish between the method at line 347 -- or if you prefer rename the latter to `allocate_with_affiliation_using_iterator()`?

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/512#pullrequestreview-2370469538
PR Review Comment: https://git.openjdk.org/shenandoah/pull/512#discussion_r1801865787
PR Review Comment: https://git.openjdk.org/shenandoah/pull/512#discussion_r1801854655
PR Review Comment: https://git.openjdk.org/shenandoah/pull/512#discussion_r1802113523
PR Review Comment: https://git.openjdk.org/shenandoah/pull/512#discussion_r1802119863