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

[Xiaolong Peng]

On Thu, 6 Mar 2025 23:52:32 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 1:
>> 
>>> 1: /*
>> 
>> This all looks good.
>> One thing to think about in general about assertions in closures is whether instead of making use of knowledge of the context in which these closures are used, whether it may produce more mantianable code to embed the "active_generation" to which the closure is being applied in the closure itself and have the assertions (or other uses of context) use that instead. Nothing to be done now, but something to think about in making more maintainable code.
>
> Right, active_generation should be used instead of global_generation to get the complete marking context, with the context of full GC, even we know it active_generation is the global gen, but it's better not to use global_generation directly for better maintainable code.

Updated it to use active_generation.

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

The method get_marking_context_for_old is called at line 1363 in method `verify_rem_set_before_mark`, as the name indicates it could be called before mark.

If current gc generation is global gc, the old marking flag should have set to false when the global flag is set, it is a bit wired I'm not sure if we should change it now, but I think we will have to correct/update the impl of this method later when update the design of completeness flags of global/young/old generations.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1984234928
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1984233610