RFR: 8327097: GenShen: Align PLAB sizes down rather than up [v6]

[Y. Srinivas Ramakrishna]

On Wed, 20 Mar 2024 19:07:52 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> When adjusting LAB sizes, round down rather than rounding up.  Otherwise, we may violate the ShenandoahHumongousThreshold bound.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add comments to clarify behavior of retire_plab

A few minor suggestions, nits, consts, etc.
Looks good to go otherwise. Happy to take a quick look again if you'd like after any changes stemming from these comments are done.

Sorry for the delay in getting this turned around.

src/hotspot/share/gc/shenandoah/shenandoahArguments.cpp line 222:

> 220:   if (strcmp(ShenandoahGCMode, "generational") == 0) {
> 221:     ShenandoahGenerationalHeap* heap = new ShenandoahGenerationalHeap(new ShenandoahCollectorPolicy());
> 222:     CollectedHeap* result = heap;

Wonder if it might read cleaner if we reversed the if-condition and returned the ShenandoahHeap from inside the if, then did the more cumbersome GenShen thing after.

Also, did you need to save in CollectedHeap* result to return it?
Simpler may be to do a


      ShenandoahGenerationalHeap* const gen_heap = new ...; 
      ... set min and max to card-aligned values ...
      return gen_heap;

Does that cause a compiler error?

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.hpp line 46:

> 44:   inline size_t plab_min_size() { return _min_plab_size; }
> 45:   inline size_t plab_max_size() { return _max_plab_size; }
> 46: 

If we never expect to change min and max, I'd make them const, and set them in the constructor.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1103:

> 1101:   assert(mode()->is_generational(), "PLABs only relevant to generational GC");
> 1102:   ShenandoahGenerationalHeap* generational_heap = (ShenandoahGenerationalHeap*) this;
> 1103:   size_t plab_min_size = generational_heap->plab_min_size();

const

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1109:

> 1107:   } else {
> 1108:     min_size = plab_min_size;
> 1109:   }

May be more succint:

const size_t min_size = align_up(MAX2(size, plab_min_size), CardTable::card_size_in_words());

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1115:

> 1113:   // available evacuation budget between the many threads that are coordinating in the evacuation effort.
> 1114:   future_size = MIN2(future_size, generational_heap->plab_max_size());
> 1115:   assert (future_size % CardTable::card_size_in_words() == 0, "Should align by design");

You can use

assert(is_aligned(future_size, CardTable::card_size_in_words(), "Aligned by construction");

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1117:

> 1115:   if (cur_size == 0) {
> 1116:     cur_size = plab_min_size;
> 1117:   }

More succinct:

const size_t cur_size = MAX2(plab_min_size, ShenandoahThreadLocalData::plab_size(thread));
assert(is_aligned(cur_size, CardTable::card_size_in_words()), "Sanity check, by construction");

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1124:

> 1122:   assert(is_aligned(future_size, CardTable::card_size_in_words()),
> 1123:          "Align by design, future_size: " SIZE_FORMAT ", card_size: " SIZE_FORMAT ", max_size: " SIZE_FORMAT,
> 1124:          future_size, (size_t) CardTable::card_size_in_words(), generational_heap->plab_max_size());

More succinct:

const size_t future_size = MIN2(cur_size*2, generational_heap->plab_max_size());
...

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1165:

> 1163: #endif // ASSERT
> 1164:     }
> 1165:     assert (actual_size % CardTable::card_size_in_words() == 0,

Can use `is_aligned(actual_size, CardTable::...)` inside assert, avoiding `%`.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1211:

> 1209:     unexpend_promoted(not_promoted);
> 1210:   }
> 1211:   size_t original_waste = plab->waste();

const

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp line 46:

> 44: 
> 45:   HeapWord* orig_top = top();
> 46:   size_t addr_as_int = (uintptr_t) orig_top;

addr_as_int isn't needed anymore, and can be deleted?

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp line 52:

> 50:   // from alignment_in_words to determine padding required to next alignment point.
> 51: 
> 52:   HeapWord* aligned_obj = (HeapWord*) align_up(orig_top, alignment_in_bytes);

I think this method's declaration `allocate_aligned(...)` in the `.hpp` uses `alignment_in_words` as the name of the second parm. Similarly the comment in the header. Those should be corrected too to say `alignment_in_bytes`.

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp line 54:

> 52:   HeapWord* aligned_obj = (HeapWord*) align_up(orig_top, alignment_in_bytes);
> 53:   size_t pad_words = aligned_obj - orig_top;
> 54:   if (pad_words > 0) {

Can probably just && and fold together the two if tests.

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/401#pullrequestreview-1948127476
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1531613299
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1531614502
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533002685
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533003029
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1531625382
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533005767
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533010939
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1531626379
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533012067
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533018129
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533046178
PR Review Comment: https://git.openjdk.org/shenandoah/pull/401#discussion_r1533040863