RFR: 8332548: GenShen: Factor generational mode out of gc helpers

[Y. Srinivas Ramakrishna]

On Mon, 20 May 2024 22:16:16 GMT, William Kemper <wkemper at openjdk.org> wrote:

> Some of the generational mode support in `shenandoahConcurrentGC.cpp` and `shenandoahDegeneratedGC.cpp` can be factored into generational mode specific files and coalesced under fewer generational mode checks.

Changes look mostly good to me. Just some very minor comments and a check about brackets in diff in one place.

Approving it, since any changes there should not require my rereview.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 789:

> 787:   JavaThread* const jt = JavaThread::cast(thread);
> 788:   StackWatermarkSet::finish_processing(jt, _oops, StackWatermarkKind::gc);
> 789:   if (ShenandoahHeap::heap()->mode()->is_generational()) {

For patterns such as this, I wonder if the closure might just be enhanced with a `_generational_mode` boolean (or templatized, but that is usually ugly and makes debugging a pain) to do these kinds of checks, and whether it makes a difference from a readability or performance perspective. Not important but thought a uniform approach might make it easier. I personally prefer a const value embedded in closures nicer, but it would show up as a diff in legacy Shenandoah (which might still be acceptable to reviewers, I think). Anyway, nothing to actually do now, but leaving this comment here for people to think/dicsuss about from a maintainability and readability perspective for the future....

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp line 175:

> 173:         // and this degenerated cycle. These pointers need to be included the 'read' table
> 174:         // used to scan the remembered set during the STW mark which follows here.
> 175:         _generation->merge_write_table();

Interesting. I am trying to understand this diff (the change in nesting looks curious because I didn't see anything change above. As a result, this specifically generational work(?) may have been pulled into the `else {` arm of the if-then-else at lines 145-149, introducing a bug?

Or is this intentional and I am missing some subtle reasoning here?

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp line 286:

> 284: 
> 285:       if (heap->mode()->is_generational()) {
> 286:         ShenandoahGenerationalHeap::heap()->complete_degenerated_cycle();

Not a change that you made, but I wondered about the semantics of "complete" in the name "op_cleanup_complete". It appears to use a phase timer around its work. Was the idea to include the "complete_degenerated_cycle" work inside that phase for timers associated with whatever `ShenandoahPhaseTimings::degen_gc_cleanup_complete` wanted to track, or is it good as is to exclude that generational work which may be is separately tracked?

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.hpp line 111:

> 109:   TransferResult balance_generations();
> 110: 
> 111:   // Balance generations, coalesce and fill old regions if necessary

To keep with surrounding documentation style, use the form "Balances generations, coalesces and fills ... etc."

The same comment might apply at line 53 further up too, but doesn't matter since there are no other comments nearby wrt which it would look inconsistent (in either the prescriptive or descriptive style).

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/436#pullrequestreview-2069167961
PR Review Comment: https://git.openjdk.org/shenandoah/pull/436#discussion_r1608710998
PR Review Comment: https://git.openjdk.org/shenandoah/pull/436#discussion_r1608694281
PR Review Comment: https://git.openjdk.org/shenandoah/pull/436#discussion_r1608683130
PR Review Comment: https://git.openjdk.org/shenandoah/pull/436#discussion_r1608662149