8260497: Shenandoah: Improve SATB flushing

Reviewed-by: shade, zgu

Author: rkennke

src/hotspot/share/gc/shared/satbMarkQueue.hpp

@@ -111,7 +111,7 @@ class SATBMarkQueueSet: public PtrQueueSet {
 
   // Return true if the queue's buffer should be enqueued, even if not full.
   // The default method uses the buffer enqueue threshold.
-  virtual bool should_enqueue_buffer(SATBMarkQueue& queue);
+  bool should_enqueue_buffer(SATBMarkQueue& queue);
 
   template<typename Filter>
   void apply_filter(Filter filter, SATBMarkQueue& queue);

src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp

@@ -31,6 +31,7 @@
 #include "gc/shared/weakProcessor.inline.hpp"
 #include "gc/shared/gcTimer.hpp"
 #include "gc/shared/gcTrace.hpp"
+#include "gc/shared/satbMarkQueue.hpp"
 #include "gc/shared/strongRootsScope.hpp"
 
 #include "gc/shenandoah/shenandoahBarrierSet.inline.hpp"
@@ -232,18 +233,46 @@ void ShenandoahConcurrentMark::mark_concurrent_roots() {
   workers->run_task(&task);
 }
 
+class ShenandoahFlushSATBHandshakeClosure : public HandshakeClosure {
+private:
+  SATBMarkQueueSet& _qset;
+public:
+  ShenandoahFlushSATBHandshakeClosure(SATBMarkQueueSet& qset) :
+    HandshakeClosure("Shenandoah Flush SATB Handshake"),
+    _qset(qset) {}
+
+  void do_thread(Thread* thread) {
+    _qset.flush_queue(ShenandoahThreadLocalData::satb_mark_queue(thread));
+  }
+};
+
 void ShenandoahConcurrentMark::concurrent_mark() {
   ShenandoahHeap* const heap = ShenandoahHeap::heap();
   WorkGang* workers = heap->workers();
   uint nworkers = workers->active_workers();
   task_queues()->reserve(nworkers);
 
-  {
+  ShenandoahSATBMarkQueueSet& qset = ShenandoahBarrierSet::satb_mark_queue_set();
+  ShenandoahFlushSATBHandshakeClosure flush_satb(qset);
+  for (uint flushes = 0; flushes < ShenandoahMaxSATBBufferFlushes; flushes++) {
     TaskTerminator terminator(nworkers, task_queues());
     ShenandoahConcurrentMarkingTask task(this, &terminator);
     workers->run_task(&task);
-  }
 
+    if (heap->cancelled_gc()) {
+      // GC is cancelled, break out.
+      break;
+    }
+
+    size_t before = qset.completed_buffers_num();
+    Handshake::execute(&flush_satb);
+    size_t after = qset.completed_buffers_num();
+
+    if (before == after) {
+      // No more retries needed, break out.
+      break;
+    }
+  }
   assert(task_queues()->is_empty() || heap->cancelled_gc(), "Should be empty when not cancelled");
 }
 

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp

@@ -56,7 +56,6 @@ ShenandoahControlThread::ShenandoahControlThread() :
   reset_gc_id();
   create_and_start();
   _periodic_task.enroll();
-  _periodic_satb_flush_task.enroll();
   if (ShenandoahPacing) {
     _periodic_pacer_notify_task.enroll();
   }
@@ -71,10 +70,6 @@ void ShenandoahPeriodicTask::task() {
   _thread->handle_counters_update();
 }
 
-void ShenandoahPeriodicSATBFlushTask::task() {
-  ShenandoahHeap::heap()->force_satb_flush_all_threads();
-}
-
 void ShenandoahPeriodicPacerNotify::task() {
   assert(ShenandoahPacing, "Should not be here otherwise");
   ShenandoahHeap::heap()->pacer()->notify_waiters();

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

@@ -46,13 +46,6 @@ class ShenandoahPeriodicTask : public PeriodicTask {
   virtual void task();
 };
 
-// Periodic task to flush SATB buffers periodically.
-class ShenandoahPeriodicSATBFlushTask : public PeriodicTask {
-public:
-  ShenandoahPeriodicSATBFlushTask() : PeriodicTask(ShenandoahSATBBufferFlushInterval) {}
-  virtual void task();
-};
-
 // Periodic task to notify blocked paced waiters.
 class ShenandoahPeriodicPacerNotify : public PeriodicTask {
 public:
@@ -77,7 +70,6 @@ class ShenandoahControlThread: public ConcurrentGCThread {
   Monitor _alloc_failure_waiters_lock;
   Monitor _gc_waiters_lock;
   ShenandoahPeriodicTask _periodic_task;
-  ShenandoahPeriodicSATBFlushTask _periodic_satb_flush_task;
   ShenandoahPeriodicPacerNotify _periodic_pacer_notify_task;
 
 public:

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

@@ -1684,20 +1684,6 @@ void ShenandoahHeap::prepare_update_heap_references(bool concurrent) {
   _update_refs_iterator.reset();
 }
 
-void ShenandoahHeap::force_satb_flush_all_threads() {
-  if (!is_concurrent_mark_in_progress()) {
-    // No need to flush SATBs
-    return;
-  }
-
-  for (JavaThreadIteratorWithHandle jtiwh; JavaThread *t = jtiwh.next(); ) {
-    ShenandoahThreadLocalData::set_force_satb_flush(t, true);
-  }
-  // The threads are not "acquiring" their thread-local data, but it does not
-  // hurt to "release" the updates here anyway.
-  OrderAccess::fence();
-}
-
 void ShenandoahHeap::set_gc_state_all_threads(char state) {
   for (JavaThreadIteratorWithHandle jtiwh; JavaThread *t = jtiwh.next(); ) {
     ShenandoahThreadLocalData::set_gc_state(t, state);

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

@@ -580,7 +580,6 @@ class ShenandoahHeap : public CollectedHeap {
 
   // SATB barriers hooks
   inline bool requires_marking(const void* entry) const;
-  void force_satb_flush_all_threads();
 
   // Support for bitmap uncommits
   bool commit_bitmap_slice(ShenandoahHeapRegion *r);

src/hotspot/share/gc/shenandoah/shenandoahSATBMarkQueueSet.cpp

@@ -53,20 +53,3 @@ void ShenandoahSATBMarkQueueSet::filter(SATBMarkQueue& queue) {
   ShenandoahHeap* heap = ShenandoahHeap::heap();
   apply_filter(ShenandoahSATBMarkQueueFilterFn(heap), queue);
 }
-
-bool ShenandoahSATBMarkQueueSet::should_enqueue_buffer(SATBMarkQueue& queue) {
-  if (SATBMarkQueueSet::should_enqueue_buffer(queue)) {
-    return true;
-  } else if (queue.index() < buffer_size()) { // Is buffer not empty?
-    Thread* t = Thread::current();
-    if (ShenandoahThreadLocalData::is_force_satb_flush(t)) {
-      // Non-empty buffer is compacted, and we decided not to enqueue it.
-      // We still want to know about leftover work in that buffer eventually.
-      // This avoid dealing with these leftovers during the final-mark, after
-      // the buffers are drained completely. See JDK-8205353 for more discussion.
-      ShenandoahThreadLocalData::set_force_satb_flush(t, false);
-      return true;
-    }
-  }
-  return false;
-}

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

@@ -31,9 +31,6 @@
 #include "runtime/thread.hpp"
 
 class ShenandoahSATBMarkQueueSet : public SATBMarkQueueSet {
-protected:
-  virtual bool should_enqueue_buffer(SATBMarkQueue& queue);
-
 public:
   ShenandoahSATBMarkQueueSet(BufferNode::Allocator* allocator);
 

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

@@ -48,7 +48,6 @@ class ShenandoahThreadLocalData {
   PLAB* _gclab;
   size_t _gclab_size;
   uint  _worker_id;
-  bool _force_satb_flush;
   int  _disarmed_value;
   double _paced_time;
 
@@ -60,7 +59,6 @@ class ShenandoahThreadLocalData {
     _gclab(NULL),
     _gclab_size(0),
     _worker_id(INVALID_WORKER_ID),
-    _force_satb_flush(false),
     _disarmed_value(0),
     _paced_time(0) {
 
@@ -115,14 +113,6 @@ class ShenandoahThreadLocalData {
     return data(thread)->_worker_id;
   }
 
-  static void set_force_satb_flush(Thread* thread, bool v) {
-    data(thread)->_force_satb_flush = v;
-  }
-
-  static bool is_force_satb_flush(Thread* thread) {
-    return data(thread)->_force_satb_flush;
-  }
-
   static void initialize_gclab(Thread* thread) {
     assert (thread->is_Java_thread() || thread->is_Worker_thread(), "Only Java and GC worker threads are allowed to get GCLABs");
     assert(data(thread)->_gclab == NULL, "Only initialize once");

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

@@ -330,9 +330,9 @@
           "Number of entries in an SATB log buffer.")                       \
           range(1, max_uintx)                                               \
                                                                             \
-  product(uintx, ShenandoahSATBBufferFlushInterval, 100, EXPERIMENTAL,      \
-          "Forcefully flush non-empty SATB buffers at this interval. "      \
-          "Time is in milliseconds.")                                       \
+  product(uintx, ShenandoahMaxSATBBufferFlushes, 5, EXPERIMENTAL,           \
+          "How many times to maximum attempt to flush SATB buffers at the " \
+          "end of concurrent marking.")                                     \
                                                                             \
   product(bool, ShenandoahSuspendibleWorkers, false, EXPERIMENTAL,          \
           "Suspend concurrent GC worker threads at safepoints")             \