8310062: [Shenandoah] Incomplete SATB buffers may not be processed during degenerated young collection

Reviewed-by: ysr, kdnilsen

Author: William Kemper

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() &&
@@ -156,9 +147,20 @@ 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_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();
+        }
       }
 
       if (_degen_point == ShenandoahDegenPoint::_degenerated_roots) {

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

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;
@@ -597,6 +599,20 @@ class ShenandoahVerifierReachableTask : public WorkerTask {
   }
 };
 
+class ShenandoahVerifyNoIncompleteSatbBuffers : public ThreadClosure {
+public:
+  virtual void do_thread(Thread* thread) {
+    SATBMarkQueue& queue = ShenandoahThreadLocalData::satb_mark_queue(thread);
+    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 {
 private:
   const char* _label;
@@ -622,6 +638,10 @@ class ShenandoahVerifierMarkedRegionTask : public WorkerTask {
           _claimed(0),
           _processed(0),
           _generation(nullptr) {
+    if (_options._verify_marked == ShenandoahVerifier::_verify_marked_complete_satb_empty) {
+      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 +653,11 @@ class ShenandoahVerifierMarkedRegionTask : public WorkerTask {
   }
 
   virtual void work(uint worker_id) {
+    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,
                                   ShenandoahMessageBuffer("%s, Marked", _label),
@@ -945,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);
@@ -1031,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

src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp

@@ -88,7 +88,12 @@ class ShenandoahVerifier : public CHeapObj<mtGC> {
 
     // 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 {