RFR: 8325671: Shenandoah: Introduce a ShenandoahGenerationType and templatize certain marking closures with it [v4]

[Aleksey Shipilev]

On Wed, 14 Feb 2024 18:16:16 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> 8325671: Shenandoah: Introduce a ShenandoahGenerationType and templatize certain marking closures with it
>
> Y. Srinivas Ramakrishna has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into generation_type
>  - Merge branch 'master' into generation_type
>  - Missing #include of ShenandoahGenerationType (albeit satisfied transitively via other include).
>  - Merge branch 'master' into generation_type
>  - Introduce ShenandoahGenerationType and templatize most closures with it.
>    The template expands for only the NON_GEN type for the non-generational
>    version of Shenandoah currently, and will in the future accomodate
>    Generational Shenandoah.

The change looks okay, but I do not see a good reason to introduce this into Shenandoah before Generational mode arrives. It is just extra (mostly dead) code to maintain, and it effectively hides what changes Generational needs to do in shared Shenandoah code. Maybe we should consider this as part of Generational integration PR?

src/hotspot/share/gc/shenandoah/shenandoahMark.cpp line 92:

> 90: 
> 91: template<bool CANCELLABLE, StringDedupMode STRING_DEDUP>
> 92: void ShenandoahMark::mark_loop(ShenandoahGenerationType generation /* ignored */, uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp, StringDedup::Requests* const req) {

I think `/* ignored */` is not needed here.

Also, the argument list line is probably too long. AFAICS, we put the arguments we curry from method arguments to template arguments at very end. See how the method arguments are entering the template arguments in the same order:


template<bool CANCELLABLE, StringDedupMode STRING_DEDUP>
void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
                               ShenandoahGenerationType generation, StringDedup::Requests* const req) ... 

void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
                               bool cancellable, StringDedupMode dedup_mode, ShenandoahGenerationType generation, StringDedup::Requests* const req ...

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

PR Review: https://git.openjdk.org/jdk/pull/17815#pullrequestreview-1882776311
PR Review Comment: https://git.openjdk.org/jdk/pull/17815#discussion_r1491016785