RFR: 8351091: Shenandoah: global marking context completeness is not accurately maintained [v7]

[Y. Srinivas Ramakrishna]

On Mon, 31 Mar 2025 23:09:53 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> With the JEP 404: Generational Shenandoah implementation, there are generation specific marking completeness flags introduced, and the global marking context completeness flag is not updated at all after initialization, hence the global marking context completeness is not accurate anymore. This may cause expected behavior: [ShenandoahHeap::complete_marking_context()](https://github.com/openjdk/jdk/pull/23886/files#diff-d5ddf298c36b1c91bf33f9bff7bedcc063074edd68c298817f1fdf39d2ed970fL642) should throw assert error if the global marking context completeness flag is false, but now it always return the marking context even it marking is not complete, this may hide bugs where we expect the global/generational marking to be completed.
>> 
>> This change PR fix the bug in global marking context completeness flag, and update all the places using `ShenandoahHeap::complete_marking_context()` to use proper API.
>> 
>> ### Test
>> - [x] hotspot_gc_shenandoah
>> - [x] Tier 1
>> - [x]  Tier 2
>
> Xiaolong Peng has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Can't verify marked object with complete marking after full GC

I looked at the files that changed since the last review only, but can look over all of it once again if necessary (just let me know).

This looks good; just a few small comments, and in particular a somewhat formalistic and pedantic distinction between the use of `gc_generation()` and `active_generation()` to fetch the marking context (and the use of `global_generation()`).

Otherwise looks good to me.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 352:

> 350:     assert(_from_region != nullptr, "must set before work");
> 351:     assert(_heap->active_generation()->complete_marking_context()->is_marked(p), "must be marked");
> 352:     assert(!_heap->active_generation()->complete_marking_context()->allocated_after_mark_start(p), "must be truly marked");

I am probably being a bit pedantic here...

I would use `gc_generation()` in all code that is executed strictly by GC threads, and `active_generation()` in all code that may possibly be executed by a mutator thread. It seems as if today this code is only executed by GC threads.

In general, there is no real distinction between these field at times like these (STW pauses) when heap verification is taking place, but from a syntactic hygiene perspective.

We can otherwise file a ticket to separately clean up any confusion in the use of these fields (and add a dynamic check to prevent creeping confusion). The names aren't super well-chosen, but generally think of `_gc_generation` as the generation that is being GC'd, `_active_generation` as one that mutator threads are aware is being the subject of GC. Any assertions by mutator threads should use the latter and by GC threads the former. The fields are reconciled at STW pauses.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 778:

> 776:   ShenandoahAdjustPointersClosure() :
> 777:     _heap(ShenandoahHeap::heap()),
> 778:     _ctx(ShenandoahHeap::heap()->global_generation()->complete_marking_context()) {}

I liked the changes in this file that everywhere use the heap's `_gc_generation` (see comment about the distinction between `gc_generation()` and `active_generation()` above) field to fetch the marking context. While I understand that it might be the case that whenever we are here, the `_gc_generation` must necessarily be the `global_generation()`, I am wondering about:

1. using `_gc_generation` here as well to fetch the context, and
2. secondly, asserting also that the `_gc_generation` is in fact the `global_generation()`.

I assume (2) must be the case here? If not, it would be good to see if this can be fixed.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 1094:

> 1092:     ShenandoahHeapRegion* region = _regions.next();
> 1093:     ShenandoahHeap* heap = ShenandoahHeap::heap();
> 1094:     ShenandoahMarkingContext* const ctx = heap->global_generation()->complete_marking_context();

Same comment as at line 778.

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 1191:

> 1189:           _verify_remembered_after_full_gc,  // verify read-write remembered set
> 1190:           _verify_forwarded_none,      // all objects are non-forwarded
> 1191:           _verify_marked_incomplete,   // all objects are marked in incomplete bitmap

Is the marking bitmap updated as objects are moved to their new locations? Is that done just to satisfy the verifier?

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

PR Review: https://git.openjdk.org/jdk/pull/23886#pullrequestreview-2741111545
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r2027772698
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r2027710108
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r2027713065
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r2027777968