8369392: Safepoint sync should suspend GC and Java threads concurrently

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -1570,11 +1570,15 @@ void G1CollectedHeap::stop() {
 }
 
 void G1CollectedHeap::safepoint_synchronize_begin() {
-  SuspendibleThreadSet::synchronize();
+  SuspendibleThreadSet::synchronize_begin();
 
   _last_synchronized_start = os::elapsed_counter();
 }
 
+void G1CollectedHeap::safepoint_synchronize() {
+  SuspendibleThreadSet::synchronize();
+}
+
 void G1CollectedHeap::safepoint_synchronize_end() {
   jlong now = os::elapsed_counter();
   jlong synchronize_duration = now - _last_synchronized_start;

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

@@ -910,6 +910,9 @@ class G1CollectedHeap : public CollectedHeap {
   jint initialize() override;
 
   void safepoint_synchronize_begin() override;
+
+  void safepoint_synchronize() override;
+
   void safepoint_synchronize_end() override;
 
   jlong last_refinement_epoch_start() const { return _last_refinement_epoch_start; }

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp

@@ -257,6 +257,7 @@ bool G1ConcurrentRefineSweepState::swap_gc_threads_ct() {
         // For example in the rebuild remset process the marking threads write
         // marks into the card table, and that card table reference must be the
         // correct one.
+        SuspendibleThreadSet::synchronize_begin();
         SuspendibleThreadSet::synchronize();
         SuspendibleThreadSet::desynchronize();
       };

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp

@@ -153,6 +153,12 @@ void ParallelScavengeHeap::initialize_serviceability() {
 }
 
 void ParallelScavengeHeap::safepoint_synchronize_begin() {
+  if (UseStringDeduplication) {
+    SuspendibleThreadSet::synchronize_begin();
+  }
+}
+
+void ParallelScavengeHeap::safepoint_synchronize() {
   if (UseStringDeduplication) {
     SuspendibleThreadSet::synchronize();
   }

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp

@@ -160,6 +160,9 @@ class ParallelScavengeHeap : public CollectedHeap {
   jint initialize() override;
 
   void safepoint_synchronize_begin() override;
+
+  void safepoint_synchronize() override;
+
   void safepoint_synchronize_end() override;
 
   void post_initialize() override;

src/hotspot/share/gc/serial/serialHeap.cpp

@@ -146,6 +146,12 @@ GrowableArray<MemoryPool*> SerialHeap::memory_pools() {
 }
 
 void SerialHeap::safepoint_synchronize_begin() {
+  if (UseStringDeduplication) {
+    SuspendibleThreadSet::synchronize_begin();
+  }
+}
+
+void SerialHeap::safepoint_synchronize() {
   if (UseStringDeduplication) {
     SuspendibleThreadSet::synchronize();
   }

src/hotspot/share/gc/serial/serialHeap.hpp

@@ -259,6 +259,9 @@ class SerialHeap : public CollectedHeap {
                            OldGenScanClosure* old_cl);
 
   void safepoint_synchronize_begin() override;
+
+  void safepoint_synchronize() override;
+
   void safepoint_synchronize_end() override;
 
   // Support for loading objects from CDS archive into the heap

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

@@ -257,6 +257,7 @@ class CollectedHeap : public CHeapObj<mtGC> {
 
   // Stop and resume concurrent GC threads interfering with safepoint operations
   virtual void safepoint_synchronize_begin() {}
+  virtual void safepoint_synchronize() {}
   virtual void safepoint_synchronize_end() {}
 
   void add_vmthread_cpu_time(jlong time);

src/hotspot/share/gc/shared/suspendibleThreadSet.cpp

@@ -24,12 +24,14 @@
 
 #include "gc/shared/gc_globals.hpp"
 #include "gc/shared/suspendibleThreadSet.hpp"
+#include "logging/log.hpp"
 #include "runtime/javaThread.hpp"
 #include "runtime/mutexLocker.hpp"
 #include "runtime/semaphore.hpp"
 
 uint          SuspendibleThreadSet::_nthreads          = 0;
 uint          SuspendibleThreadSet::_nthreads_stopped  = 0;
+volatile bool SuspendibleThreadSet::_has_synchronized  = false;
 volatile bool SuspendibleThreadSet::_suspend_all       = false;
 double        SuspendibleThreadSet::_suspend_all_start = 0.0;
 
@@ -89,19 +91,27 @@ void SuspendibleThreadSet::yield_slow() {
   }
 }
 
-void SuspendibleThreadSet::synchronize() {
+void SuspendibleThreadSet::synchronize_begin() {
   if (ConcGCYieldTimeout > 0) {
     _suspend_all_start = os::elapsedTime();
   }
+
   {
     MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag);
     assert(!should_yield(), "Only one at a time");
     AtomicAccess::store(&_suspend_all, true);
     if (is_synchronized()) {
-      return;
+      AtomicAccess::store(&_has_synchronized, true);
     }
-  } // Release lock before semaphore wait.
+  }
+}
 
+void SuspendibleThreadSet::synchronize() {
+  assert(AtomicAccess::load(&_suspend_all), "synchronize must have begun.");
+  // If STS has synchronized when synchronize begin, all suspendible threads will be waiting on STS_lock.
+  if (AtomicAccess::load(&_has_synchronized)) {
+    return;
+  }
   // Semaphore initial count is zero.  To reach here, there must be at
   // least one not yielded thread in the set, e.g. is_synchronized()
   // was false before the lock was released.  A thread in the set will
@@ -114,7 +124,10 @@ void SuspendibleThreadSet::synchronize() {
   // being signaled until we get back here again for some later
   // synchronize call.  Hence, there is no need to re-check for
   // is_synchronized after the wait; it will always be true there.
+  log_trace(safepoint)("Waiting for %d suspendible threads to block", _nthreads - _nthreads_stopped);
   _synchronize_wakeup->wait();
+  AtomicAccess::store(&_has_synchronized, true);
+  log_trace(safepoint)("All suspendible threads have blocked");
 
 #ifdef ASSERT
   MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag);
@@ -127,6 +140,7 @@ void SuspendibleThreadSet::desynchronize() {
   MonitorLocker ml(STS_lock, Mutex::_no_safepoint_check_flag);
   assert(should_yield(), "STS not synchronizing");
   assert(is_synchronized(), "STS not synchronized");
+  AtomicAccess::store(&_has_synchronized, false);
   AtomicAccess::store(&_suspend_all, false);
   ml.notify_all();
 }

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

@@ -43,6 +43,7 @@ class SuspendibleThreadSet : public AllStatic {
 private:
   static uint          _nthreads;
   static uint          _nthreads_stopped;
+  static volatile bool _has_synchronized;
   static volatile bool _suspend_all;
   static double        _suspend_all_start;
 
@@ -68,7 +69,10 @@ class SuspendibleThreadSet : public AllStatic {
     }
   }
 
-  // Returns when all threads in the set are suspended.
+  // begin to synchronize suspendible threads,
+  static void synchronize_begin();
+
+  // synchronize all suspendible threads
   static void synchronize();
 
   // Resumes all suspended threads in the set.

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

@@ -2699,6 +2699,10 @@ bool ShenandoahHeap::is_uncommit_in_progress() {
 
 void ShenandoahHeap::safepoint_synchronize_begin() {
   StackWatermarkSet::safepoint_synchronize_begin();
+  SuspendibleThreadSet::synchronize_begin();
+}
+
+void ShenandoahHeap::safepoint_synchronize() {
   SuspendibleThreadSet::synchronize();
 }
 

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

@@ -666,6 +666,9 @@ class ShenandoahHeap : public CollectedHeap {
 //
 public:
   void safepoint_synchronize_begin() override;
+
+  void safepoint_synchronize() override;
+
   void safepoint_synchronize_end() override;
 
 // ---------- Code roots handling hooks

src/hotspot/share/gc/z/zCollectedHeap.cpp

@@ -333,6 +333,10 @@ bool ZCollectedHeap::contains_null(const oop* p) const {
 
 void ZCollectedHeap::safepoint_synchronize_begin() {
   StackWatermarkSet::safepoint_synchronize_begin();
+  SuspendibleThreadSet::synchronize_begin();
+}
+
+void ZCollectedHeap::safepoint_synchronize() {
   ZGeneration::young()->synchronize_relocation();
   ZGeneration::old()->synchronize_relocation();
   SuspendibleThreadSet::synchronize();

src/hotspot/share/gc/z/zCollectedHeap.hpp

@@ -110,6 +110,9 @@ class ZCollectedHeap : public CollectedHeap {
   bool contains_null(const oop* p) const override;
 
   void safepoint_synchronize_begin() override;
+
+  void safepoint_synchronize() override;
+
   void safepoint_synchronize_end() override;
 
   void pin_object(JavaThread* thread, oop obj) override;

src/hotspot/share/gc/z/zGeneration.cpp

@@ -1311,6 +1311,7 @@ class ZRendezvousGCThreads: public VM_Operation {
 
   void doit() {
     // Light weight "handshake" of the GC threads
+    SuspendibleThreadSet::synchronize_begin();
     SuspendibleThreadSet::synchronize();
     SuspendibleThreadSet::desynchronize();
   };

src/hotspot/share/runtime/safepoint.cpp

@@ -371,6 +371,7 @@ void SafepointSynchronize::begin() {
   log_trace(safepoint)("Arming safepoint using %s wait barrier", _wait_barrier->description());
   arm_safepoint();
 
+  Universe::heap()->safepoint_synchronize();
   // Will spin until all threads are safe.
   int iterations = synchronize_threads(safepoint_limit_time, nof_threads, &initial_running);
   assert(_waiting_to_block == 0, "No thread should be running");

src/hotspot/share/runtime/synchronizer.cpp

@@ -1069,6 +1069,7 @@ class VM_RendezvousGCThreads : public VM_Operation {
   VMOp_Type type() const override { return VMOp_RendezvousGCThreads; }
   void doit() override {
     Universe::heap()->safepoint_synchronize_begin();
+    Universe::heap()->safepoint_synchronize();
     Universe::heap()->safepoint_synchronize_end();
   };
 };