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

[Xiaolong Peng]

On Thu, 6 Mar 2025 22:05:31 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> Xiaolong Peng has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Remove ShenandoahHeap::complete_marking_context(ShenandoahHeapRegion* region)
>>  - Revert "complete_marking_context should guarantee mark is complete"
>>    
>>    This reverts commit 2004973965ea0e617cf9e5fc45be24f0e06e90a1.
>>  - complete_marking_context should guarantee mark is complete
>
> src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp line 172:
> 
>> 170: // contained herein.
>> 171: void ShenandoahGenerationalEvacuationTask::promote_in_place(ShenandoahHeapRegion* region) {
>> 172:   ShenandoahMarkingContext* const marking_context = _heap->young_generation()->complete_marking_context();
> 
> For clarity, you might assert the following before line 172:
> 
>     assert(gc_generation() == _heap->young_generation(), "Sanity check");
> 
> 
> Even though it might seem somewhat tautological.

Thanks, I'll add it.

> src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 1283:
> 
>> 1281:   if (_heap->gc_generation()->is_global()) {
>> 1282:     return _heap->marking_context();
>> 1283:   }
> 
> Not sure I understand the point of this change in behavior. What purpose does a partial marking context serve? Why not just leave the behavior as was before and return a non-null marking context only when marking is complete and null otherwise. When the client uses the context, it does so to skip over unmarked objects (which are dead if marking is complete), which might end up being too weak if we are still in the midst of marking.
> 
> I realize that you may not be maintaining a global mark completion so you are returning the marking context irrespective of the state of completion of the marking, but I wonder if that is really the bahavior you want. I would rather, as necessary, we maintain a flag for completion of global marking for the case where we are doing a global gc?

It is confusing that it looks like a behavior change, but actually there is no behavior change in this method, all the change here is to make the behavior of this method to be exactly same a before. 

The old impl always return the the marking context, regardless the completeness status of marking, because the old `_heap->complete_marking_context()` always return w/o assert error due to inaccurate completeness marking status in the marking context, we are fixing the issue in this PR which breaks the old impl of this method.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1984188328
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1984188121