RFR: 8319867: GenShen: Make old regions parseable at end of concurrent cycles

[Y. Srinivas Ramakrishna]

On Thu, 9 Nov 2023 21:35:40 GMT, William Kemper <wkemper at openjdk.org> wrote:

> ### Background
> When Shenandoah is performing a concurrent mark of the old generation, it is not safe for the young generation's remembered set scan to use the old mark bitmap. For this reason, any old regions that were not included in a collection set (either mixed or a global collection) must be made _parseable_ for the remembered set scan (we call this "coalescing and filling" in the code).
> 
> ### Description
> Before this change, Shenandoah has deferred making these regions parseable until it begins the young generation bootstrap cycle. This is fine, except that we have also recently made old generation collections subordinate to young collections. In other words, an old collection cannot start unless Shenandoah wants to first start a young collection.
> 
> Unfortunately, the additional time to concurrently coalesce and fill old regions before resetting the old mark bitmaps is not accounted for in the heuristics and this increases the likelihood that the heuristic has started the cycles too late and that the cycle will degenerate into a stop the world pause.
> 
> After this change, Shenandoah will make old regions parseable immediately following the last mixed collection (or following a global collection). At this point, Shenandoah has just finished reclaiming memory and we expect less urgency to coalesce and fill old regions. It also means that young bootstrap cycles will not take (much) longer than conventional young generation collections.

Some remarks, not all of them need to be immediately actioned (if the remarks are correct).

Was there a test case that revealed this? If so, can we add that to the JBS ticket and mentioned as part of the testing for the PR?

Marked as reviewed by ysr (Committer).

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.hpp line 93:

> 91:   // both record_full() and record_degenerated() with the same value of gcid.  record_full() is called first and the log
> 92:   // reports such a cycle as a FULL cycle.
> 93:   void record_young(ShenandoahGeneration* generation, size_t gcid);

Thanks for cleaning up the unused ShenandoahGeneration* and other unused parms.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 259:

> 257: }
> 258: 
> 259: // Make the old generation regions parseable, so they can be safely

Nothing to do now, but my ex-colleague Peter Kessler told me many years ago that the correct spelling is "parsable" (without the "e" between the s and the a :-) One can do a cleanup of spelling in a separate PR in the fullness of time.

https://word.tips/spelling/parsable-vs-parseable/

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 428:

> 426:   switch (new_state) {
> 427:     case FILLING:
> 428:       assert(heap->is_old_bitmap_stable(), "Cannot begin filling without first completing marking, state is '%s'", state_name(_state));

Do you also want to say:


assert(_state > FILLING && _state != WAITING_FOR_BOOTSTRAP, "Illegal state transition"):

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 432:

> 430:       break;
> 431:     case WAITING_FOR_BOOTSTRAP:
> 432:       // GC cancellation can send us back to IDLE from any state.

Since there isn't now a state named "IDLE", it might be less confusing to just say "FILLING" or "WAITING_FOR_BOOTSTRAP"? Should we assert here that there are no `coalesce_and_fill_candidates` or is that implied by perhaps stronger checks below? (Wouldn't hurt even if so.)

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 448:

> 446:       break;
> 447:     case EVACUATING:
> 448:       assert(_state == WAITING_FOR_BOOTSTRAP || _state == MARKING, "Cannot have old collection candidates without first marking, state is '%s'", state_name(_state));

FWIW, the ASCII art state-transition-diagram doesn't show the transition from WAITING_FOR_BOOTSTRAP directly into EVACUATING, without passing through an intermediate MARKING.

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

PR Review: https://git.openjdk.org/shenandoah/pull/355#pullrequestreview-1725807183
PR Review: https://git.openjdk.org/shenandoah/pull/355#pullrequestreview-1725836333
PR Review Comment: https://git.openjdk.org/shenandoah/pull/355#discussion_r1390033021
PR Review Comment: https://git.openjdk.org/shenandoah/pull/355#discussion_r1390037239
PR Review Comment: https://git.openjdk.org/shenandoah/pull/355#discussion_r1390050334
PR Review Comment: https://git.openjdk.org/shenandoah/pull/355#discussion_r1390047203
PR Review Comment: https://git.openjdk.org/shenandoah/pull/355#discussion_r1390053016