RFR: 8313574: GenShen: Completing a global mark should also complete an old mark [v2]

[Y. Srinivas Ramakrishna]

On Mon, 14 Aug 2023 19:47:17 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> The mark context is shared, but each generation maintains a separate flag (`ShenandoahGeneration::_is_marking_complete`) to record the status of the mark. The remembered set scan will use the mark bitmap only if the old generation mark is complete. Otherwise, it walks the old regions and requires them to be parseable.
>> 
>> Presently, we make the heap parseable after global marking finishes, but this work can be deferred to the start of an old mark if we had global mark record completion of old marking.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Merge branch 'shenandoah-master' into global-completes-old-mark
>  - Fix assertions and update comments
>  - Prepare for mixed collections after global cycle
>  - Merge branch upstream into global-completes-old-mark
>  - Have global mark also complete young and old marks
>    
>    This will have the remembered set scan use the mark bitmap following a global mark and eliminates the need to coalesce and fill during a global collection.

Left a few comments, and going over all of the changes again to make sure I understand the changes completely again before I sign off on the review.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp line 149:

> 147:   unsigned int get_coalesce_and_fill_candidates(ShenandoahHeapRegion** buffer);
> 148:   bool has_coalesce_and_fill_candidates() const { return _last_old_region > 0; }
> 149:   size_t coalesce_and_fill_candidates() const { return _last_old_region; }

The return value from `coalesce_and_fill_candidates()` (which by the way could be confused for an imperative action verb) lready available in `last_old_region_index()` although might be a good idea to rename appropriately? Also fine to leave as is, but it sounds to me like one name might work for both uses.

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

> 103:       assert(!heap->is_old_gc_active(), "Old GC should not be active during global cycle");
> 104:     }
> 105: 

Is this not true?

You could rearrange the test into the assertion check though:


    assert(!_generation->is_global() || !heap->is_old_gc_active, "Old GC should not be active during global cycle");

(or its de-Morganization, if you prefer).

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

> 415: //           |   |        |     +-----------------+
> 416: //           |   |        |     |    WAITING FOR  |
> 417: //           |   +--------|---> |    EVACUATIONS  |

This transition has arrows at both ends (IDLE and WAITING FOR EVACUATIONS).

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

PR Review: https://git.openjdk.org/shenandoah/pull/304#pullrequestreview-1577748885
PR Review Comment: https://git.openjdk.org/shenandoah/pull/304#discussion_r1294028628
PR Review Comment: https://git.openjdk.org/shenandoah/pull/304#discussion_r1294031288
PR Review Comment: https://git.openjdk.org/shenandoah/pull/304#discussion_r1294041386