RFR: 8331609: GenShen: Refactor generational mode allocations [v4]

[Y. Srinivas Ramakrishna]

On Wed, 8 May 2024 23:23:28 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> * The code for resizing and retrying young allocation requests according to what's available in the young generation has been removed. This code became redundant after the free set was rewritten and began providing the same functionality.
>> * An unused field and its attendant methods were removed from `ShenandoahThreadLocalData`.
>> * The code for managing `plab` expenditures and configuring other plab behavior has been moved from `ShenandoahHeap` into `ShenandoahOldGeneration`.
>
> William Kemper has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Improve comments, remove too strong assert
>  - Initialize _evacuation_reserve member
>  - Modify test fixture to avoid accessing ShenandoahHeap before we know if UseShenandoahGC is set

A few minor comments; overall looks good, but is still a bit difficult to reason about because of having to carry several case analyses simultaneously in these allocation paths.

Was there any performance difference measured in any of the code pipeline performance tests on account of avoiding some of the redundant tests that you removed?

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

> 1115:   ShenandoahHeapLocker locker(lock(), req.is_mutator_alloc());
> 1116: 
> 1117:   // Make sure the old generation has room for either evacuations or promotions before trying to allocate.

I'd rephrase this more directly:


// Don't try allocation if there's no room, or if promotion budget exhausted.

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

> 1121: 
> 1122:   // If TLAB request size is greater than available, allocate() will attempt to downsize request to fit within available
> 1123:   // memory.

I am confused. Why do we need this comment here? Could this not be a non-TLAB allocation request? Why are we specifically talking about TLABs here?

If we believe this can only be a TLAB, we should assert that `req.type() == _alloc_tlab` ?

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

> 1145:       // The thread allocating b and the thread allocating c can "race" in various ways, resulting in confusion, such as
> 1146:       // last-start representing object b while first-start represents object c.  This is why we need to require all
> 1147:       // register_object() invocations to be "mutually exclusive" with respect to each card's memory range.

One can simply and more succinctly state:


  // Register with object start array while holding heap lock to protect against
  // update races between threads allocating to the same card.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 247:

> 245:   // This is a shared allocation request. We've already checked that it can't be promoted, so if
> 246:   // it is a promotion, we return false. Otherwise, it is a shared evacuation request, and we allow
> 247:   // the allocation to proceed.

I'd rephrase to:
// We already dealt with PLAB allocations above. If it isn't a promotion, we'll
// allow the allocation attempt, because it must be a shared allocation.

Can we assert here that this is `_is_promotion` or else `_alloc_shared` or `_alloc_shared_gc` just to be sure we catch any changes to this or calling code in the future?

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 271:

> 269:         ShenandoahThreadLocalData::set_plab_actual_size(thread, actual_size);
> 270:       } else {
> 271:         // Disable promotions in this thread because entirety of this PLAB must be available to hold old-gen evacuations.

I would rephrase, removing the "because" :


  // Disable promotions in this thread's PLAB. The entirety of the PLAB
  // will be used for old gen evacuations.


I must say I find the flow of allocation logic in this and earlier code difficult to clearly understand and reason about, because a lot of allocation paths are aggregated into common code, so one needs to continually carry a lot of case analyses through the control flow.

Addition of assertions (as stated control point invariants) might help readability and comprehension, if refactoring for clarity isn't a good idea. (But any of this for the future.)

src/hotspot/share/gc/shenandoah/shenandoahThreadLocalData.hpp line 64:

> 62:   PLAB* _plab;
> 63: 
> 64:   // Heuristics will grow the desired size of plabs.

~"grow" or "adjust" (i.e. are downwards adjustments also done?)~ Scratch that, I see that it only grows starting at initial size.

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

PR Review: https://git.openjdk.org/shenandoah/pull/427#pullrequestreview-2048797700
PR Review Comment: https://git.openjdk.org/shenandoah/pull/427#discussion_r1596072178
PR Review Comment: https://git.openjdk.org/shenandoah/pull/427#discussion_r1596074282
PR Review Comment: https://git.openjdk.org/shenandoah/pull/427#discussion_r1596121597
PR Review Comment: https://git.openjdk.org/shenandoah/pull/427#discussion_r1596068015
PR Review Comment: https://git.openjdk.org/shenandoah/pull/427#discussion_r1596109882
PR Review Comment: https://git.openjdk.org/shenandoah/pull/427#discussion_r1596023955