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

[Xiaolong Peng]

On Wed, 5 Mar 2025 17:59:36 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

> 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!

Thanks, I'll update PR to address your comments.

> 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?

It may not be needed anymore, I will double check the usage and remove it is not used at all.

> 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 :-)

Yes, the assert at line 350 should use complete_marking_context, I have update it in the fix of the issue we found in stress test.

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

PR Comment: https://git.openjdk.org/jdk/pull/23886#issuecomment-2702079928
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1982190515
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1982188076