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

[Y. Srinivas Ramakrishna]

On Tue, 4 Mar 2025 08:34:16 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

Had a few questions and comments inline. I'll take a closer look once you have responded to those.

Thank you for finding this probably long-standing incorrectness/fuzziness and fixing it properly!

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1028:

> 1026: 
> 1027: #ifdef ASSERT
> 1028:     ShenandoahMarkingContext* const ctx = _heap->marking_context();

Why not this instead?

 ShenandoahMarkingContext* const ctx = _heap->marking_context(r);

src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp line 206:

> 204:   bool is_mark_complete() { return _is_marking_complete.is_set(); }
> 205:   virtual void set_mark_complete();
> 206:   virtual void set_mark_incomplete();

Why are these declared virtual?

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 737:

> 735: public:
> 736:   inline ShenandoahMarkingContext* complete_marking_context(ShenandoahHeapRegion* region) const;
> 737:   inline ShenandoahMarkingContext* marking_context() const;

Should document semantics of both methods, please!

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp line 868:

> 866: #ifdef ASSERT
> 867:   {
> 868:     // During full gc, heap->complete_marking_context() is not valid, may equal nullptr.

Looks like this comment is obsolete?

src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 103:

> 101: 
> 102: bool ShenandoahMarkingContext::is_complete() {
> 103:   return ShenandoahHeap::heap()->global_generation()->is_mark_complete();

Do we need this? It seems wrong to me that even though each generation has its own marking context, we ask any marking context to report if that of the Global Generation is complete. I'd explicitly let generations maintain the state of completeness of their marking contexts, and for clients to query the generations for that state rather than having the individual marking contexts respond to that question.

Where is this used after your changes?

src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp line 88:

> 86:   bool is_bitmap_range_within_region_clear(const HeapWord* start, const HeapWord* end) const;
> 87: 
> 88:   bool is_complete();

Add a 1-line documentation comment for this method.

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 337:

> 335:   // drop the reference.
> 336:   if (type == REF_PHANTOM) {
> 337:     return heap->complete_marking_context(referent_region)->is_marked(raw_referent);

Doesn't the assert down at line 350 also need `complete_marking_context` ? Same at line 441. May be comb through all of these to determine which we need for proper assertion checking?

I'd start by documenting the semantics of the APIs clearly. I am not completely clear on that yet (pun not intended :-)

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

PR Review: https://git.openjdk.org/jdk/pull/23886#pullrequestreview-2659389355
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980523168
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980420417
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980401312
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980403403
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980437298
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980406186
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1981905245