RFR: Borrow from old gen

[William Kemper]

On Fri, 4 Feb 2022 22:52:01 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

> This commit represents multiple months of incremental performance improvements to allow generational shenandoah to run more efficiently, especially with larger heap sizes and high memory utilization.
> 
> Specific improvements are described in individual commit log messages.

Changes requested by wkemper (Committer).

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 341:

> 339:   size_t penultimate_live_memory = get_penultimate_live_memory();
> 340:   double original_cycle_time = avg_cycle_time;
> 341:   if ((penultimate_live_memory < last_live_memory) && (penultimate_live_memory != 0)) {

The heuristic uses an exponentially weighted average for cycle times. The decay factor can be adjusted from the command line. The margin of error for estimating cycle times is also adjusted in response to degenerated cycles or successful concurrent cycles. Perhaps we should have the heuristic also adjust the decay factor? Have we evaluated the results of these changes on a variety of work loads?

src/hotspot/share/gc/shenandoah/shenandoahCollectionSet.hpp line 50:

> 48:   size_t                _used;
> 49:   size_t                _region_count;
> 50:   size_t                _immediate_trash;

Might consider having collection set itself manage these new members through `add_region` API - rather than having similar looking code in each of the heuristics when the collection set is populated.

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

> 118:       // GCLABs are for evacuation so we must be in evacuation phase.  If this allocation is successful, increment
> 119:       // the relevant evac_expended rather than used value.
> 120:       allocating_gclab = true;

Do we really need a new parameter here? The information is carried in the type of the allocation request (`req`) which is also passed to `allocate_with_affiliation` (the request goes all the way down to `try_allocate_in`).

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

> 80:   ShenandoahFreeSet(ShenandoahHeap* heap, size_t max_regions);
> 81: 
> 82:   // How many regions dedicated to GC allocations (for evacuation or promotion) are currently free?

I agree with these comments - let's delete the question marks?

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 362:

> 360:   // At the end of update references, we perform the following bookkeeping activities:
> 361:   //
> 362:   // 1. Unadjust the capacity within young-gen and old-gen

Does "unadjust" mean we remove the evacuation reserve from the capacity?

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 385:

> 383: 
> 384:   size_t _old_evac_reserve;            // Bytes reserved within old-gen to hold evacuated objects from old-gen collection set
> 385:   size_t _old_evac_expended;           // Bytes of old-gen memory expended on old-gen evacuations?

Why do these descriptive comments end with a question mark?

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 394:

> 392:                // the desired PLAB size and retry PLAB allocation to avoid cascading of shared memory allocations.
> 393:                ShenandoahThreadLocalData::set_plab_size(thread, PLAB::min_size());
> 394:                copy = allocate_from_gclab(thread, size);

Should this be `allocate_from_plab` as above?

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp line 253:

> 251: void ShenandoahHeapRegion::make_cset() {
> 252:   shenandoah_assert_heaplocked();
> 253:   // Leave age untouched.  We need to consult the age when we are deciding whether to promote evacuated objects.

Many other state transitions also `reset_age` and I'm not sure it's necessary. Seems sufficient to do it only for `make_trash`.

src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.cpp line 74:

> 72:         r->reset_age();
> 73:       } else if (ShenandoahHeap::heap()->is_aging_cycle()) {
> 74:         r->increment_age();

Hmm, we used to increment age for every region during final mark, but now we are doing it during final update refs. This means no regions will be aged for cycles that skip evacuation. Is that what we want? this will have the effect of lowering the promotion rate.

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

> 174:   }
> 175: 
> 176:   static void disable_plab_promotions(Thread* thread) {

This looks wrong - shouldn't it set `_plab_allows_promotion` to false here? It's also not clear to me how this relates to the other changes here. When would we disable plab promotion?

src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 309:

> 307:           range(1,100)                                                      \
> 308:                                                                             \
> 309:   product(uintx, ShenandoahOldEvacRatioPer128, 16, EXPERIMENTAL,            \

These `Per128` options are evaluated once per cycle? I would just take the floating point hit for a more user friendly percentage.

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

PR: https://git.openjdk.java.net/shenandoah/pull/110