RFR: 8342001: GenShen: Factor cases for allocation type into separate methods [v2]

[Y. Srinivas Ramakrishna]

On Mon, 14 Oct 2024 18:55:13 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Prune switch statement which has grown unwieldy.
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve comments for new methods

Refactoring looks good to me. Left a few comments that may or may not help, but raised a few questions in my mind. These aren't changes you made, but looking at the change raised questions that may have been raised and addressed earlier perhaps. (cc @kdnilsen).

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

> 760: 
> 761: HeapWord* ShenandoahFreeSet::allocate_for_mutator(ShenandoahAllocRequest &req, bool &in_new_region) {
> 762:   maybe_change_allocation_bias();

Why update alloc bias if allocation id doomed to fail at next line? Is that the intention? If not, may be do the bias update only when there is chance for success in the allocation? See suggestions further below.

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

> 772:   }
> 773: 
> 774:   return allocate_from_right_to_left(req, in_new_region);

Just a thought: Might make the code easier to read, maintain, and reason about, if instead of a boolean value indicating left bias, if we used a two value enum: LeftBias, RightBias, and used an `_alloc_bias` array for the partitions.

Then code might read:

    .... set_alloc_bias(LeftBias);   // set allocation bias to left

and the above code might look like:

   if (_partitions.is_empy(SFSPI::Mutator)) {
     return nullptr;
   }
   update_alloc_bias();
   switch (_partitions._alloc_bias[ShenandoahPartitionId::Mutator]) {
     case LeftBias:  return allocate_from_left(req, in_new_region);
     case RightBias: return allocate_from_right(req, in_new_region);
     default: ShouldNotReachHere();
   }

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

> 775: }
> 776: 
> 777: void ShenandoahFreeSet::maybe_change_allocation_bias() {

Since this is a counting update, I'd rename it to `update_allocation_bias()`.

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

> 797:                                 - _partitions.rightmost_empty(ShenandoahFreeSetPartitionId::Mutator));
> 798:     _partitions.set_bias_from_left_to_right(ShenandoahFreeSetPartitionId::Mutator, (non_empty_on_right < non_empty_on_left));
> 799:     _alloc_bias_weight = _InitialAllocBiasWeight;

The value of 256 for `_InitialAllocBiasWeight` seems a bit of magic. How was it chosen, and is it subject to bit-rot with code evolving? I can imagine that the bias update at a higher frequency is apt to not matter much, and since the leftmost and rightmost empty calculation is not free, so larger values are favored.

Also should we use the more usual `static const ssize_t INITIAL_ALLOC_BIAS_WEIGHT` Hotspot style for constants, rather than the camel case with an initial underscore (which seems not common)?

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

> 811:            "Boundaries or find_last_set_bit failed: " SSIZE_FORMAT, idx);
> 812:     ShenandoahHeapRegion* r = _heap->get_region(idx);
> 813:     // try_allocate_in() increases used if the allocation is successful.

I think the comment about what `try_allocate_in()` does belongs as a spec comment for that method, rather than here. I assume `used` in this comment refers to the used bytes for the (here, mutator) partition?

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

> 830:            "Boundaries or find_last_set_bit failed: " SSIZE_FORMAT, idx);
> 831:     ShenandoahHeapRegion* r = _heap->get_region(idx);
> 832:     // try_allocate_in() increases used if the allocation is successful.

Same comment re "used".

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

> 897: }
> 898: 
> 899: HeapWord* ShenandoahFreeSet::try_allocate_from_mutator(ShenandoahAllocRequest& req, bool& in_new_region) {

Why does this always use right-biased allocation from the mutator partition? (And not update allocation bias state?) Is this intentional? I assume it's because these aren't fresh allocations, but evacuations. Do evacuations with right bias end up closer to allocations for other evacuations? A brief comment explaining this difference from mutator allocation would be useful.

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

> 332:   HeapWord* allocate_for_mutator(ShenandoahAllocRequest &req, bool &in_new_region);
> 333: 
> 334:   // Chose whether to change the allocation bias from the left or right side of the heap.

Chose -> Choose

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

> 333: 
> 334:   // Chose whether to change the allocation bias from the left or right side of the heap.
> 335:   void maybe_change_allocation_bias();

Weak suggestion above: `update_allocation_bias()`

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/511#pullrequestreview-2367465853
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800019810
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800016996
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800008130
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800048082
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800074385
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800077212
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800147723
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1799987970
PR Review Comment: https://git.openjdk.org/shenandoah/pull/511#discussion_r1800112075