RFR: 8368152: Shenandoah: Incorrect behavior at end of degenerated cycle [v3]

[Y. Srinivas Ramakrishna]

On Tue, 23 Sep 2025 21:24:20 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> There are several issues addressed in this PR:
>> * Shenandoah always ran a full GC after any degenerated cycle
>> * The number of consecutive degenerated GCs with bad progress was reset for every degenerated cycle
>> * Good progress was reported in generational mode even when no progress is made
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve comment

Looks good; left some documentation nits, but approved even if the documentation suggestions aren't taken.

🚢

src/hotspot/share/gc/shenandoah/shenandoahCollectorPolicy.hpp line 68:

> 66:   // in generational mode, run a full GC. Non-generational modes will upgrade
> 67:   // immediately when a degenerated cycle does not make progress. Many degenerated
> 68:   // cycles are caused by floating garbage. It is more efficient to attempt

If it's used only for generational case, I'd like the comment to be less coy about it. I'd place `GENERATIONAL` or some such in the name of the constant, e.g. `GENERATIONAL_TO_FULL_NON_PROGRESSING_DEGENS`,  and its "accessor" (or user) down in `should_upgrade_degenerated_gc()` to, e.g., `generational_should_upgrade_degen_to_full()`.

The comment could say:


// When a degenerated cycle doesn't make progress:
// a. for the non-generational case, we'll immediately upgrade to full at the first non-progressing degen
// b. for the generational case, we'll upgrade to full after these many non-progressing consecutive degens


My question is why we choose this number for genshen. May be a sentence in the ticket or in the comments on the intuition here?

src/hotspot/share/gc/shenandoah/shenandoahCollectorPolicy.hpp line 117:

> 115:   // Genshen will only upgrade to a full gc after the configured number of futile degenerated cycles.
> 116:   bool should_upgrade_degenerated_gc() const {
> 117:     return _consecutive_degenerated_gcs_without_progress >= CONSECUTIVE_BAD_DEGEN_PROGRESS_THRESHOLD;

assert that heap is generational? See previous comment above at line 68.

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

> 321:   // Shenandoah to avoid introducing "surprising new behavior."  It also makes less sense with non-generational
> 322:   // Shenandoah to replace a full GC with a degenerated GC, because both have similar pause times in non-generational
> 323:   // mode.

May be, based on the first paragraph above, you want to say more simply (replacing the entire second paragraph):

On the other hand, in non-generational mode,
to both preserve legacy behavior, and because the
difference between a degenerated gc and full gc is smaller,
we immediately escalate to
a full gc following the first degenerated cycle that doesn't make progress.


I'd also move this comment out from here and to the constant that was defined for the number of consecutive degens before escalating to full in generational mode.

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

Marked as reviewed by ysr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27456#pullrequestreview-3259658651
PR Review Comment: https://git.openjdk.org/jdk/pull/27456#discussion_r2373504315
PR Review Comment: https://git.openjdk.org/jdk/pull/27456#discussion_r2373507843
PR Review Comment: https://git.openjdk.org/jdk/pull/27456#discussion_r2373546896