RFR: Enable remembered set verification during global collections

[Kelvin Nilsen]

On Wed, 8 Sep 2021 18:55:19 GMT, William Kemper <wkemper 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/shenandoahConcurrentGC.cpp line 1044:

> 1042: void ShenandoahConcurrentGC::op_global_coalesce_and_fill() {
> 1043:   ShenandoahHeap* const heap = ShenandoahHeap::heap();
> 1044:   ShenandoahGlobalCoalesceAndFill coalesce;

It looks like this is iterating over all old regions.  Not clear to me how we avoid coalescing and filling the regions that are part of the collection set.   I think our intent was to not coalesce-and-fill regions in the collection set.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2202:

> 2200:         } else if (r->affiliation() == ShenandoahRegionAffiliation::OLD_GENERATION) {
> 2201:           if (_heap->active_generation()->generation_mode() == GLOBAL) {
> 2202:             _heap->marked_object_oop_iterate(r, &cl, update_watermark);

In the original code, we used the same iterator over marked objects for both coalescing and filling and for updating references.  That is "more efficient" than this new approach in that we make one pass instead of two over all objects in all heap regions.  I understand that there was a bug in how that is implemented, apparently, because this new version of the code has fewer crashes than the original.  Nevertheless, would like to have a comment here that suggests we might explore a way to consolidate the efforts at some future time, especially if we detect performance regressions with this new approach.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2639:

> 2637:         while (obj_addr < t) {
> 2638:           oop obj = oop(obj_addr);
> 2639:           // ctx->is_marked() returns true if mark bit set or if obj above TAMS.

The reason original comment said "TAMS not relevant here" is because this verification occurs at init_mark.  There have been no allocations above TAMS at this execution point.  (Or if there is, maybe add a comment to explain how that comes to be.)

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2657:

> 2655:             HeapWord* tams = ctx->top_at_mark_start(r);
> 2656:             if (obj_addr >= tams) {
> 2657:               obj_addr += obj->size();

Were you actually seeing obj_addr >= tams here?  I'd be inclined to replace your new code with an assert(obj_addr < tams).  Or provide comments to explain.

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.

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.

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?

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.

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

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