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

[Xiaolong Peng]

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

>> I am not sure I follow. In the legacy (non-generational mode) we shouldn't care about the marking state of the old and young generations, just that of the GlobalGeneration. In the generational case, we explicitly track the marking state of the old and young generations explicitly. It sounds to me as if forcing the Old and Young marking states to the state of that of the GlobalGeneration must be exactly for the case where we are using Generational Shenandoah, and we are doing a Global collection? Indeed:
>> 
>> 
>> void ShenandoahGlobalGeneration::set_mark_complete() {
>>   ShenandoahGeneration::set_mark_complete();
>>   if (ShenandoahHeap::heap()->mode()->is_generational()) {
>>     ShenandoahGenerationalHeap* heap = ShenandoahGenerationalHeap::heap();
>>     heap->young_generation()->set_mark_complete();
>>     heap->old_generation()->set_mark_complete();
>>   }
>> }
>> 
>> 
>> I am saying that each of Old, Young, and Global generations maintain their own mark completion state and use that to determine what they pass back in response to `complete_marking_context()`. This completely localizes all state rather than unnecessarily and confusingly coupling the states of these generations.
>> 
>> So, you remove the part in the `if` branch in the code above, which reduces to the default (or rather only) implementation in the base class, not requiring the over-ride of the Global generation's method for the generational case.
>> 
>> 
>> void ShenandoahGeneration::set_mark_complete() {
>>   _is_marking_complete.set();
>> }
>> 
>> 
>> It is possible that I am still missing the actual structure here that requires the override for GlobalGeneration for the generational case.
>
> Sorry I misunderstood your original proposal, I thought you meant to suggest to remove the flag from  ShenandoahGlobalGeneration, instead the set_mark_complete/is_mark_complete will more like view/delegation layer like:
> 
> void ShenandoahGlobalGeneration::set_mark_complete() {
>     ShenandoahGenerationalHeap* heap = ShenandoahGenerationalHeap::heap();
>     heap->young_generation()->set_mark_complete();
>     heap->old_generation()->set_mark_complete();
> }
> 
> bool ShenandoahGlobalGeneration::is_mark_complete() {
>     ShenandoahGenerationalHeap* heap = ShenandoahGenerationalHeap::heap();
>     return heap->young_generation()->is_mark_complete() && heap->old_generation()->is_mark_complete();
> }

You proposal will make the impl of the  set_mark_complete/is_mark_complete of ShenandoahGeneration cleaner, but the thing is it will change current design and behavior, we may have to update the code where there methods is called, e.g. when we call `set_mark_complete` of  gc_generation/active_generation, if it is global generation, we  may have to explicitly call the same methods of  ShenandoahYoungGeneration and ShenandoahOldGeneration to fan out the status. 

How about I follow up it in a separate task and update the implementation if necessary? I want to limit the changes involved in this PR, and only fix the bug.

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

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