From 4f866d48056f4fe37b771c7d37a73d9f169416e2 Mon Sep 17 00:00:00 2001 From: William Kemper Date: Mon, 12 Jun 2023 17:39:34 -0700 Subject: [PATCH 1/3] Transfer old satb pointers for degenerated roots and out of cycle --- .../gc/shenandoah/shenandoahDegeneratedGC.cpp | 18 +++++++++--------- .../gc/shenandoah/shenandoahOldGeneration.hpp | 2 +- .../share/gc/shenandoah/shenandoahVerifier.cpp | 17 +++++++++++++++++ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp index 0a7354b17be..aa1401763e2 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp @@ -125,15 +125,6 @@ void ShenandoahDegenGC::op_degenerated() { // we can do the most aggressive degen cycle, which includes processing references and // class unloading, unless those features are explicitly disabled. - if (heap->is_concurrent_old_mark_in_progress()) { - // We have come straight into a degenerated cycle without running a concurrent cycle - // first and the SATB barrier is enabled to support concurrent old marking. The SATB buffer - // may hold a mix of old and young pointers. The old pointers need to be transferred - // to the old generation mark queues and the young pointers are _not_ part of this - // snapshot, so they must be dropped here. - heap->transfer_old_pointers_from_satb(); - } - // Note that we can only do this for "outside-cycle" degens, otherwise we would risk // changing the cycle parameters mid-cycle during concurrent -> degenerated handover. heap->set_unload_classes(_generation->heuristics()->can_unload_classes() && @@ -159,6 +150,15 @@ void ShenandoahDegenGC::op_degenerated() { // the entire old gc before coming into this switch. _generation->cancel_marking(); } + + if (heap->is_concurrent_old_mark_in_progress()) { + assert(!_generation->is_global(), "Old marking should not be in progress for global collection"); + // If old marking is in progress, the SATB barrier will be enabled. The SATB buffer + // may hold a mix of old and young pointers. The old pointers need to be transferred + // to the old generation mark queues and the young pointers are _not_ part of this + // snapshot, so they must be dropped here. + heap->transfer_old_pointers_from_satb(); + } } if (_degen_point == ShenandoahDegenPoint::_degenerated_roots) { diff --git a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp index 1ac7e22c41a..484804afddc 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp @@ -65,7 +65,7 @@ class ShenandoahOldGeneration : public ShenandoahGeneration { // We leave the SATB barrier on for the entirety of the old generation // marking phase. In some cases, this can cause a write to a perfectly // reachable oop to enqueue a pointer that later becomes garbage (because - // it points at an object in the collection set, for example). There are + // it points at an object that is later chosen for the collection set). There are // also cases where the referent of a weak reference ends up in the SATB // and is later collected. In these cases the oop in the SATB buffer becomes // invalid and the _next_ cycle will crash during its marking phase. To diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp index 61ea96999d0..db0686302f9 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp @@ -597,6 +597,16 @@ class ShenandoahVerifierReachableTask : public WorkerTask { } }; +class ShenandoahVerifyNoIncompleteSatbBuffers : public ThreadClosure { +public: + virtual void do_thread(Thread* thread) { + SATBMarkQueue& queue = ShenandoahThreadLocalData::satb_mark_queue(thread); + if (queue.buffer() != nullptr) { + fatal("All SATB buffers should have been flushed during mark"); + } + }; +}; + class ShenandoahVerifierMarkedRegionTask : public WorkerTask { private: const char* _label; @@ -622,6 +632,9 @@ class ShenandoahVerifierMarkedRegionTask : public WorkerTask { _claimed(0), _processed(0), _generation(nullptr) { + + Threads::change_thread_claim_token(); + if (_heap->mode()->is_generational()) { _generation = _heap->active_generation(); assert(_generation != nullptr, "Expected active generation in this mode."); @@ -633,6 +646,10 @@ class ShenandoahVerifierMarkedRegionTask : public WorkerTask { } virtual void work(uint worker_id) { + + ShenandoahVerifyNoIncompleteSatbBuffers verify_satb; + Threads::possibly_parallel_threads_do(true, &verify_satb); + ShenandoahVerifierStack stack; ShenandoahVerifyOopClosure cl(&stack, _bitmap, _ld, ShenandoahMessageBuffer("%s, Marked", _label), From aec91667f74d14d6cae76a6b4b6ee58211f4e5fd Mon Sep 17 00:00:00 2001 From: William Kemper Date: Tue, 13 Jun 2023 16:33:36 -0700 Subject: [PATCH 2/3] Drain satb for old and young marking, fix test for ptr queue emptiness --- .../gc/shenandoah/shenandoahDegeneratedGC.cpp | 16 +++++++++------- .../share/gc/shenandoah/shenandoahVerifier.cpp | 8 ++++++-- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp index aa1401763e2..43bc1fcd53e 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp @@ -147,16 +147,18 @@ void ShenandoahDegenGC::op_degenerated() { if (_generation->is_concurrent_mark_in_progress()) { // We want to allow old generation marking to be punctuated by young collections // (even if they have degenerated). If this is a global cycle, we'd have cancelled - // the entire old gc before coming into this switch. + // the entire old gc before coming into this switch. Note that cancel_marking on + // the generation does NOT abandon incomplete SATB buffers as cancel_concurrent_mark does. + // We need to separate out the old pointers which is done below. _generation->cancel_marking(); } - if (heap->is_concurrent_old_mark_in_progress()) { - assert(!_generation->is_global(), "Old marking should not be in progress for global collection"); - // If old marking is in progress, the SATB barrier will be enabled. The SATB buffer - // may hold a mix of old and young pointers. The old pointers need to be transferred - // to the old generation mark queues and the young pointers are _not_ part of this - // snapshot, so they must be dropped here. + if (heap->is_concurrent_mark_in_progress()) { + // If either old or young marking is in progress, the SATB barrier will be enabled. + // The SATB buffer may hold a mix of old and young pointers. The old pointers need to be + // transferred to the old generation mark queues and the young pointers are NOT part + // of this snapshot, so they must be dropped here. It is safe to drop them here because + // we will rescan the roots on this safepoint. heap->transfer_old_pointers_from_satb(); } } diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp index db0686302f9..38bf3a24489 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp @@ -601,10 +601,14 @@ class ShenandoahVerifyNoIncompleteSatbBuffers : public ThreadClosure { public: virtual void do_thread(Thread* thread) { SATBMarkQueue& queue = ShenandoahThreadLocalData::satb_mark_queue(thread); - if (queue.buffer() != nullptr) { + if (!is_empty(queue)) { fatal("All SATB buffers should have been flushed during mark"); } - }; + } +private: + bool is_empty(SATBMarkQueue& queue) { + return queue.buffer() == nullptr || queue.index() == queue.capacity(); + } }; class ShenandoahVerifierMarkedRegionTask : public WorkerTask { From f4b635fae39b800a28e0074496e6e499bf134bef Mon Sep 17 00:00:00 2001 From: William Kemper Date: Wed, 14 Jun 2023 11:43:04 -0700 Subject: [PATCH 3/3] Only verify SATB buffers are empty during final mark In generational mode, the SATB barrier for old marking could pick up pointers after final mark. --- .../gc/shenandoah/shenandoahVerifier.cpp | 21 ++++++++++++------- .../gc/shenandoah/shenandoahVerifier.hpp | 7 ++++++- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp index 38bf3a24489..2ff38dfa34a 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp @@ -87,6 +87,7 @@ class ShenandoahVerifyOopClosure : public BasicOopIterateClosure { _loc(nullptr), _generation(nullptr) { if (options._verify_marked == ShenandoahVerifier::_verify_marked_complete_except_references || + options._verify_marked == ShenandoahVerifier::_verify_marked_complete_satb_empty || options._verify_marked == ShenandoahVerifier::_verify_marked_disable) { set_ref_discoverer_internal(new ShenandoahIgnoreReferenceDiscoverer()); } @@ -257,6 +258,7 @@ class ShenandoahVerifyOopClosure : public BasicOopIterateClosure { "Must be marked in complete bitmap"); break; case ShenandoahVerifier::_verify_marked_complete_except_references: + case ShenandoahVerifier::_verify_marked_complete_satb_empty: check(ShenandoahAsserts::_safe_all, obj, _heap->complete_marking_context()->is_marked_or_old(obj), "Must be marked in complete bitmap, except j.l.r.Reference referents"); break; @@ -636,8 +638,9 @@ class ShenandoahVerifierMarkedRegionTask : public WorkerTask { _claimed(0), _processed(0), _generation(nullptr) { - - Threads::change_thread_claim_token(); + if (_options._verify_marked == ShenandoahVerifier::_verify_marked_complete_satb_empty) { + Threads::change_thread_claim_token(); + } if (_heap->mode()->is_generational()) { _generation = _heap->active_generation(); @@ -650,9 +653,10 @@ class ShenandoahVerifierMarkedRegionTask : public WorkerTask { } virtual void work(uint worker_id) { - - ShenandoahVerifyNoIncompleteSatbBuffers verify_satb; - Threads::possibly_parallel_threads_do(true, &verify_satb); + if (_options._verify_marked == ShenandoahVerifier::_verify_marked_complete_satb_empty) { + ShenandoahVerifyNoIncompleteSatbBuffers verify_satb; + Threads::possibly_parallel_threads_do(true, &verify_satb); + } ShenandoahVerifierStack stack; ShenandoahVerifyOopClosure cl(&stack, _bitmap, _ld, @@ -966,7 +970,10 @@ void ShenandoahVerifier::verify_at_safepoint(const char* label, // version size_t count_marked = 0; - if (ShenandoahVerifyLevel >= 4 && (marked == _verify_marked_complete || marked == _verify_marked_complete_except_references)) { + if (ShenandoahVerifyLevel >= 4 && + (marked == _verify_marked_complete || + marked == _verify_marked_complete_except_references || + marked == _verify_marked_complete_satb_empty)) { guarantee(_heap->marking_context()->is_complete(), "Marking context should be complete"); ShenandoahVerifierMarkedRegionTask task(_verification_bit_map, ld, label, options); _heap->workers()->run_task(&task); @@ -1052,7 +1059,7 @@ void ShenandoahVerifier::verify_after_concmark() { "After Mark", _verify_remembered_disable, // do not verify remembered set _verify_forwarded_none, // no forwarded references - _verify_marked_complete_except_references, + _verify_marked_complete_satb_empty, // bitmaps as precise as we can get, except dangling j.l.r.Refs _verify_cset_none, // no references to cset anymore _verify_liveness_complete, // liveness data must be complete here diff --git a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp index 67b35644bf5..6fbdd8515ed 100644 --- a/src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp +++ b/src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp @@ -88,7 +88,12 @@ class ShenandoahVerifier : public CHeapObj { // Objects should be marked in "complete" bitmap, except j.l.r.Reference referents, which // may be dangling after marking but before conc-weakrefs-processing. - _verify_marked_complete_except_references + _verify_marked_complete_except_references, + + // Objects should be marked in "complete" bitmap, except j.l.r.Reference referents, which + // may be dangling after marking but before conc-weakrefs-processing. All SATB buffers must + // be empty. + _verify_marked_complete_satb_empty, } VerifyMarked; typedef enum {