RFR: 8352185: Shenandoah: Invalid logic for remembered set verification

[Kelvin Nilsen]

On Tue, 18 Mar 2025 07:14:23 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 648:
>> 
>>> 646:   assert(!heap->has_forwarded_objects(), "No forwarded objects on this path");
>>> 647: 
>>> 648:   if (heap->mode()->is_generational()) {
>> 
>> I think we do not want to change this code.  We only swap remembered set for young-gen because gen will scan the remset and reconstruct it with more updated information.  For  a global GC, we do not scan the remset and do not reconstruct it.  If we swap here, we will lose the information that is currently within the remset.
>
> Thanks for for explanation, I have been reading and trying the understand how the remembered set works in GenShen. I wasn't sure whether this is actually right.
> 
> In generational mode, if the GC cycle is global, the read table is already cleaned during reset phase, so remembered set verification from `verify_before_concmark` and `verify_before_update_refs` shouldn't work properly, I think the remembered set verification before mark and update references should be disabled, what do you think? Meanwhile, there is no need to clean read table during global cycle in generational mode.

Ok.  So we will always swap card tables, but we'll do it after verify-before-mark.  To clarify the intention, after we swap card table, the write-table is all clean, and the read table holds whatever had been gathered prior to the start of GC.  Young and bootstrap collection will update the write card table as a side effect of remembered set scanning.  Global collection will update the card table as a side effect of global marking of old objects.

>> src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 953:
>> 
>>> 951:     // pinned regions.
>>> 952:     if (!r->is_pinned()) {
>>> 953:       _heap->marking_context()->reset_top_at_mark_start(r);
>> 
>> Here, and below, I think we want to keep complete_marking_context() rather than changing to marking_context()
>
> The marking context is not complete anymore after ShenandoahMCResetCompleteBitmapTask, but ShenandoahMCResetCompleteBitmapTask only reset bitmaps for the regions w/o pinned objects, the place calling `set_mark_incomplete()` need to moved to some place after ShenandoahPostCompactClosure being executed if use complete_marking_context here.

Can we move heap_region_iterate(&post_compact) and post_compact.update_generation_usage() before heap->workers()->run_task(ShenandoahMCResetCompletedBitmaptask) so that we can use complete_marking_context here?  I'm a bit uncomfortable using an incomplete marking context as if it is complete.  (I understand "why it works" in this case, but this looks like an "accident waiting to happen" when someone comes back to modify this code in the future.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r2002169842
PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r2002179864