RFR: Enable remembered set verification during global collections

[William Kemper]

On Wed, 8 Sep 2021 21:08:24 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> There are two sets of changes here:
>>  * Fixes to the verifier itself to enable remembered set verification during global collection
>>  * Fixes to prevent the remembered set scan from parsing unmarked objects after a global collection
>> 
>> These changes pass the dacapo suite with `-XX:+ShenandoahVerify` and pass the shenandoah tier1 tests.
>
> src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 293:
> 
>> 291:         // ShenandoahHeapRegion::global_oop_iterate_and_fill_dead.  We could either mark all live memory as dirty, or could
>> 292:         // use the GLOBAL update-refs scanning of pointers to determine precisely which cards to flag as dirty.
>> 293:         if (GENERATION == YOUNG && heap->is_in_old(p)) {
> 
> I think I had added the heap->is_in(p) test because some program was sending in a non-heap value which was causing an assertion failure in is_in_old(p) during DEBUG builds (that have assertions enabled).  Maybe need to test this patch with FAST_DEBUG just to be sure we're ok with code as now written.

I changed the implementation of `is_in_old` to first test for `is_in` in an earlier patch.

> src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 956:
> 
>> 954: 
>> 955:   // Return true iff this object is "properly" registered.
>> 956:   bool verify_registration(HeapWord* address, ShenandoahMarkingContext* ctx);
> 
> There's some trickery with the value of the ctx argument that should probably be described in the API comment.  In particular, ctx is non-null iff the the marking context is "valid" for the purposes of finding live objects within old-gen memory.  It is not valid if we are doing a young collection while old-gen is concurrently marking.  When ctx is NULL, it is required that the old-gen memory has been coalesced and filled.  During mixed evacuations, we rely upon ctx within old-gen because we know that a new old-gen concurrent mark cannot begin until the previously selected evacuation candidates have all been collected.  We don't bother to coalesce-and-fill the collection candidates, but they may be scanned multiple times by young-gen (mixed) remembered set scanning while we are waiting for these candidate regions to be collected.  It's not clear to me that this patch honors all of these behaviors.

This method is called from the other rset verify methods (init-mark, update-references, after-full-gc). Here I've just made it take the `ctx` computed in the caller (rather than re-selecting it in the called method). The size parameter was unused.

> src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp line 547:
> 
>> 545:     }
>> 546:   } else {
>> 547:     // This is a mixed evacuation: rely on mark bits to identify which objects need to be properly registered
> 
> Maybe this comment is too restrictive.  I think the new idea is that we use ctx != nullptr for both mixed evac and for GLOBAL collections.  Am I right about this?

Yes, I'll update the comment to include global collections.

> src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp line 555:
> 
>> 553:       oop obj = cast_to_oop(base_addr + offset);
>> 554:       ShenandoahHeapRegion* region = heap->heap_region_containing(obj);
>> 555:       HeapWord* tams = ctx->top_at_mark_start(region);
> 
> Think you can move both these lines outside the loop, because we're not crossing region boundaries here.

Okay, wasn't sure if cards would always be aligned with regions.

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

PR: https://git.openjdk.java.net/shenandoah/pull/64