RFR: 8333750: GenShen: Only instantiate young/old generations in generational mode [v3]

[Y. Srinivas Ramakrishna]

On Fri, 7 Jun 2024 23:39:57 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> * Other modes should not instantiate or touch young/old generations.
>> * The generation sizer has been put in its own file and moved out of shHeap
>> * The logic for aging regions has been decoupled from non-generational final update refs
>
> William Kemper has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Make age and youth const
>  - Do not age regions after completing old mark
>  - Detect cancellation of old GC after final mark.

LGTM; left a few minor/optional comments, none of which require a re-review.

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

> 732:     } else {
> 733:       _heap->generation_for(r->affiliation())->increment_affiliated_region_count();
> 734:     }

Can the affiliation update be done unconditionally outside of the if-else, as when the affiliation is set at line 721, or if it needs to be done after the old region coalesce/fill/card-clearing, then after that at line 733, but unconditionally, skipping the update at line 731 for the old gen?

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

> 2042:   }
> 2043: 
> 2044:   global_generation()->cancel_marking();

Not that this matters, but there are a few methods `foo()` on the heap that do an operation on global and, in generational mode, on old and young. If possible, I'd use the same idiom in them uniformly(unless there is a good reason not to), i.e.
`ShenandoahHeap::foo() {
   global_gen->foo();
   if (generational) {
      young->foo();
      old->foo();
   }
`

In fact, you could just do this by means of a macro like `ALL_GENS(x)` which does this for you?

e.g. `set_mark_complete`, `set_mark_incomplete`, etc. (Not sure if there are several others in the same fashion, and therefore if it's worthwhile to macro-ize or not.

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

> 2367: 
> 2368: void ShenandoahSynchronizePinnedRegionStates::heap_region_do(ShenandoahHeapRegion* r) {
> 2369:   // Drop unnecessary "pinned" state from regions that does not have CP marks

May be "critical pins" instead of the more abstruse "CP marks"?

also "does" -> "do".

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

> 2406: }
> 2407: 
> 2408: void ShenandoahHeap::final_update_refs_do_regions() {

The name of this method is too generic, and doesn't tell you if and what you are doing? Is there a better possible name?

I see that it's a virtual method and in one case does pinning recalibration and in the other additionally region age updates. Is there a good name for these two? 

May be `final_update_refs_update_region_states()` analogous to the name of the timing phase, like for the `trash_cset` case below?

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

> 458:   // Final update region states
> 459:   void update_heap_region_states(bool concurrent);
> 460:   virtual void final_update_refs_do_regions();

Or, vide my suggestion above.

You could rename the current `update_heap_region_states()` to `update_state_and_trash()`, and then rename the virtual method to `update_heap_region_states()`. I realize this will cause changes to upstream legacy Shenandoah code, but makes the naming clearer.

This is a "weak" suggestion; you should do what feels better here (in terms of yr earlier point re "diminishing returns").

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

> 503:     return _old_generation;
> 504:   }
> 505: 

I presume that the presence of these methods here indicates that the "isolation factoring" still isn't complete, but I realize that's a tough bar to get to (and as you stated may be we are at a point of "diminishing benefits").

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

> 138:        if (heap->mode()->is_generational()) {
> 139:          heap->young_generation()->increment_affiliated_region_count();
> 140:        }

I assume the strict order here is on account of assertion checks in some of these methods? Otherwise, one might simplify it by moving the affiliation setting out from the sandwich of the count adjustments which are conditional on generational mode.

Ideally the count for young gen in non-generational mode would work for the generational case, and we could just adjust the old count like now and drop the adjustment of young count (lines 138-140) entirely, because they are covered by line 137.

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/444#pullrequestreview-2105412096
PR Review Comment: https://git.openjdk.org/shenandoah/pull/444#discussion_r1634024066
PR Review Comment: https://git.openjdk.org/shenandoah/pull/444#discussion_r1634058105
PR Review Comment: https://git.openjdk.org/shenandoah/pull/444#discussion_r1634061284
PR Review Comment: https://git.openjdk.org/shenandoah/pull/444#discussion_r1634093821
PR Review Comment: https://git.openjdk.org/shenandoah/pull/444#discussion_r1634110341
PR Review Comment: https://git.openjdk.org/shenandoah/pull/444#discussion_r1634111111
PR Review Comment: https://git.openjdk.org/shenandoah/pull/444#discussion_r1634113529