RFR: 8337511: Implement JEP-404: Generational Shenandoah (Experimental) [v3]

[Roman Kennke]

On Tue, 8 Oct 2024 17:20:31 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> This PR merges JEP 404, a generational mode for the Shenandoah garbage collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We would like to target JDK24 with this PR.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 478 commits:
> 
>  - Fix merge error
>  - Merge remote-tracking branch 'jdk/master' into great-genshen-pr-redux
>  - Merge remote-tracking branch 'jdk/master' into great-genshen-pr-redux
>  - Merge branch 'shenandoah/master' into great-genshen-pr-redux
>  - Merge
>  - 8341099: GenShen: assert(HAS_FWD == _heap->has_forwarded_objects()) failed: Forwarded object status is sane
>    
>    Reviewed-by: kdnilsen
>  - 8341485: GenShen: Make evac tracker a non-product feature and confine it to generational mode
>    
>    Reviewed-by: kdnilsen, ysr
>  - Merge
>  - 8341042: GenShen: Reset mark bitmaps for unaffiliated regions when preparing for a cycle
>    
>    Reviewed-by: kdnilsen
>  - 8339616: GenShen: Introduce new state to distinguish promote-in-place phase as distinct from concurrent evacuation
>    
>    Reviewed-by: kdnilsen, shade, ysr
>  - ... and 468 more: https://git.openjdk.org/jdk/compare/b9db74a6...4db1e0e1

And here comes the first part of `src/hotspot/share/gc/shenandoah` review.

src/hotspot/share/gc/shenandoah/shenandoahAffiliation.hpp line 44:

> 42:     default:
> 43:       ShouldNotReachHere();
> 44:       return "?";

This return is unreachable code (according to my IDE)

src/hotspot/share/gc/shenandoah/shenandoahAffiliation.hpp line 58:

> 56:     default:
> 57:       ShouldNotReachHere();
> 58:       return "?";

Same.

src/hotspot/share/gc/shenandoah/shenandoahCardStats.hpp line 40:

> 38:   DIRTY_SCAN_OBJS     = 6,
> 39:   ALTERNATIONS        = 7,
> 40:   MAX_CARD_STAT_TYPE  = 8

Are the numerical values relevant or what is the reason to spell them out?

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

> 44:   CARD_STAT_SCAN_RS       = 0,
> 45:   CARD_STAT_UPDATE_REFS   = 1,
> 46:   MAX_CARD_STAT_LOG_TYPE  = 2

Same here?

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

> 674:     case ShenandoahAllocRequest::_alloc_shared_gc: {
> 675:       // Fast-path: try to allocate in the collector view first
> 676:       idx_t leftmost_collector = _partitions.leftmost(ShenandoahFreeSetPartitionId::Collector);

1. The curly bracing that starts at _alloc_plab and then again at _alloc_shared_gc is really really weird.
2. The block that is enclosed by the curly braces is really huge for a switch-case.
It probably looks better if the code can be factored out into a method and be called from the switch cases? I'm not sure if this is easy to do, because there are some returns and breaks sprinkled in the block, too. But this only makes it worse.

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

Changes requested by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2361030955
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795859909
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795860226
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795888252
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795888425
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795936007