8377048: Shenandoah: shenandoahLock related improvments

Reviewed-by: kdnilsen, wkemper

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

@@ -41,9 +41,9 @@ bool ShenandoahBarrierSetNMethod::nmethod_entry_barrier(nmethod* nm) {
     return true;
   }
 
-  ShenandoahReentrantLock* lock = ShenandoahNMethod::lock_for_nmethod(nm);
+  ShenandoahNMethodLock* lock = ShenandoahNMethod::lock_for_nmethod(nm);
   assert(lock != nullptr, "Must be");
-  ShenandoahReentrantLocker locker(lock);
+  ShenandoahNMethodLocker locker(lock);
 
   if (!is_armed(nm)) {
     // Some other thread managed to complete while we were

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

@@ -136,13 +136,13 @@ public:
     assert(!nm_data->is_unregistered(), "Should not see unregistered entry");
 
     if (nm->is_unloading()) {
-      ShenandoahReentrantLocker locker(nm_data->lock());
+      ShenandoahNMethodLocker locker(nm_data->lock());
       nm->unlink();
       return;
     }
 
     {
-      ShenandoahReentrantLocker locker(nm_data->lock());
+      ShenandoahNMethodLocker locker(nm_data->lock());
 
       // Heal oops
       if (_bs->is_armed(nm)) {
@@ -154,7 +154,7 @@ public:
     }
 
     // Clear compiled ICs and exception caches
-    ShenandoahReentrantLocker locker(nm_data->ic_lock());
+    ShenandoahNMethodLocker locker(nm_data->ic_lock());
     nm->unload_nmethod_caches(_unloading_occurred);
   }
 };

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

@@ -1023,7 +1023,7 @@ public:
 
   void do_nmethod(nmethod* n) {
     ShenandoahNMethod* data = ShenandoahNMethod::gc_data(n);
-    ShenandoahReentrantLocker locker(data->lock());
+    ShenandoahNMethodLocker locker(data->lock());
     // Setup EvacOOM scope below reentrant lock to avoid deadlock with
     // nmethod_entry_barrier
     ShenandoahEvacOOMScope oom;

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

@@ -32,8 +32,8 @@
 #include "gc/shenandoah/shenandoahSimpleBitMap.hpp"
 #include "logging/logStream.hpp"
 
-typedef ShenandoahLock    ShenandoahRebuildLock;
-typedef ShenandoahLocker  ShenandoahRebuildLocker;
+typedef ShenandoahLock                           ShenandoahRebuildLock;
+typedef ShenandoahLocker<ShenandoahRebuildLock>  ShenandoahRebuildLocker;
 
 // Each ShenandoahHeapRegion is associated with a ShenandoahFreeSetPartitionId.
 enum class ShenandoahFreeSetPartitionId : uint8_t {

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

@@ -2834,3 +2834,13 @@ void ShenandoahHeap::log_heap_status(const char* msg) const {
     global_generation()->log_status(msg);
   }
 }
+
+ShenandoahHeapLocker::ShenandoahHeapLocker(ShenandoahHeapLock* lock, bool allow_block_for_safepoint) : _lock(lock) {
+#ifdef ASSERT
+  ShenandoahFreeSet* free_set = ShenandoahHeap::heap()->free_set();
+  // free_set is nullptr only at pre-initialized state
+  assert(free_set == nullptr || !free_set->rebuild_lock()->owned_by_self(), "Dead lock, can't acquire heap lock while holding free-set rebuild lock");
+  assert(_lock != nullptr, "Must not");
+#endif
+  _lock->lock(allow_block_for_safepoint);
+}

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

@@ -117,9 +117,23 @@ public:
   virtual bool is_thread_safe() { return false; }
 };
 
-typedef ShenandoahLock    ShenandoahHeapLock;
-typedef ShenandoahLocker  ShenandoahHeapLocker;
-typedef Stack<oop, mtGC>  ShenandoahScanObjectStack;
+typedef ShenandoahLock                       ShenandoahHeapLock;
+// ShenandoahHeapLocker implements locker to assure mutually exclusive access to the global heap data structures.
+// Asserts in the implementation detect potential deadlock usage with regards the rebuild lock that is present
+// in ShenandoahFreeSet.  Whenever both locks are acquired, this lock should be acquired before the
+// ShenandoahFreeSet rebuild lock.
+class ShenandoahHeapLocker : public StackObj {
+private:
+  ShenandoahHeapLock* _lock;
+public:
+  ShenandoahHeapLocker(ShenandoahHeapLock* lock, bool allow_block_for_safepoint = false);
+
+  ~ShenandoahHeapLocker() {
+    _lock->unlock();
+  }
+};
+
+typedef Stack<oop, mtGC>                     ShenandoahScanObjectStack;
 
 // Shenandoah GC is low-pause concurrent GC that uses a load reference barrier
 // for concurent evacuation and a snapshot-at-the-beginning write barrier for

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

@@ -93,7 +93,7 @@ ShenandoahSimpleLock::ShenandoahSimpleLock() {
   assert(os::mutex_init_done(), "Too early!");
 }
 
-void ShenandoahSimpleLock::lock() {
+void ShenandoahSimpleLock::lock(bool allow_block_for_safepoint) {
   _lock.lock();
 }
 
@@ -101,28 +101,31 @@ void ShenandoahSimpleLock::unlock() {
   _lock.unlock();
 }
 
-ShenandoahReentrantLock::ShenandoahReentrantLock() :
-  ShenandoahSimpleLock(), _owner(nullptr), _count(0) {
-  assert(os::mutex_init_done(), "Too early!");
+template<typename Lock>
+ShenandoahReentrantLock<Lock>::ShenandoahReentrantLock() :
+  Lock(), _owner(nullptr), _count(0) {
 }
 
-ShenandoahReentrantLock::~ShenandoahReentrantLock() {
+template<typename Lock>
+ShenandoahReentrantLock<Lock>::~ShenandoahReentrantLock() {
   assert(_count == 0, "Unbalance");
 }
 
-void ShenandoahReentrantLock::lock() {
+template<typename Lock>
+void ShenandoahReentrantLock<Lock>::lock(bool allow_block_for_safepoint) {
   Thread* const thread = Thread::current();
   Thread* const owner = _owner.load_relaxed();
 
   if (owner != thread) {
-    ShenandoahSimpleLock::lock();
+    Lock::lock(allow_block_for_safepoint);
     _owner.store_relaxed(thread);
   }
 
   _count++;
 }
 
-void ShenandoahReentrantLock::unlock() {
+template<typename Lock>
+void ShenandoahReentrantLock<Lock>::unlock() {
   assert(owned_by_self(), "Invalid owner");
   assert(_count > 0, "Invalid count");
 
@@ -130,12 +133,17 @@ void ShenandoahReentrantLock::unlock() {
 
   if (_count == 0) {
     _owner.store_relaxed((Thread*)nullptr);
-    ShenandoahSimpleLock::unlock();
+    Lock::unlock();
   }
 }
 
-bool ShenandoahReentrantLock::owned_by_self() const {
+template<typename Lock>
+bool ShenandoahReentrantLock<Lock>::owned_by_self() const {
   Thread* const thread = Thread::current();
   Thread* const owner = _owner.load_relaxed();
   return owner == thread;
 }
+
+// Explicit template instantiation
+template class ShenandoahReentrantLock<ShenandoahSimpleLock>;
+template class ShenandoahReentrantLock<ShenandoahLock>;

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

@@ -31,7 +31,7 @@
 #include "runtime/javaThread.hpp"
 #include "runtime/safepoint.hpp"
 
-class ShenandoahLock  {
+class ShenandoahLock {
 private:
   enum LockState { unlocked = 0, locked = 1 };
 
@@ -48,7 +48,7 @@ private:
 public:
   ShenandoahLock() : _state(unlocked), _owner(nullptr) {};
 
-  void lock(bool allow_block_for_safepoint) {
+  void lock(bool allow_block_for_safepoint = false) {
     assert(_owner.load_relaxed() != Thread::current(), "reentrant locking attempt, would deadlock");
 
     if ((allow_block_for_safepoint && SafepointSynchronize::is_synchronizing()) ||
@@ -83,34 +83,19 @@ public:
   }
 };
 
-class ShenandoahLocker : public StackObj {
-private:
-  ShenandoahLock* const _lock;
-public:
-  ShenandoahLocker(ShenandoahLock* lock, bool allow_block_for_safepoint = false) : _lock(lock) {
-    if (_lock != nullptr) {
-      _lock->lock(allow_block_for_safepoint);
-    }
-  }
-
-  ~ShenandoahLocker() {
-    if (_lock != nullptr) {
-      _lock->unlock();
-    }
-  }
-};
-
+// Simple lock using PlatformMonitor
 class ShenandoahSimpleLock {
 private:
   PlatformMonitor   _lock; // native lock
 public:
   ShenandoahSimpleLock();
-
-  virtual void lock();
-  virtual void unlock();
+  void lock(bool allow_block_for_safepoint = false);
+  void unlock();
 };
 
-class ShenandoahReentrantLock : public ShenandoahSimpleLock {
+// templated reentrant lock
+template<typename Lock>
+class ShenandoahReentrantLock : public Lock {
 private:
   Atomic<Thread*>       _owner;
   uint64_t              _count;
@@ -119,30 +104,25 @@ public:
   ShenandoahReentrantLock();
   ~ShenandoahReentrantLock();
 
-  virtual void lock();
-  virtual void unlock();
+  void lock(bool allow_block_for_safepoint = false);
+  void unlock();
 
   // If the lock already owned by this thread
   bool owned_by_self() const ;
 };
 
-class ShenandoahReentrantLocker : public StackObj {
-private:
-  ShenandoahReentrantLock* const _lock;
-
+// template based ShenandoahLocker
+template<typename Lock>
+class ShenandoahLocker : public StackObj {
+  Lock* const _lock;
 public:
-  ShenandoahReentrantLocker(ShenandoahReentrantLock* lock) :
-    _lock(lock) {
-    if (_lock != nullptr) {
-      _lock->lock();
-    }
+  ShenandoahLocker(Lock* lock, bool allow_block_for_safepoint = false) : _lock(lock) {
+    assert(_lock != nullptr, "Must not");
+    _lock->lock(allow_block_for_safepoint);
   }
 
-  ~ShenandoahReentrantLocker() {
-    if (_lock != nullptr) {
-      assert(_lock->owned_by_self(), "Must be owner");
-      _lock->unlock();
-    }
+  ~ShenandoahLocker() {
+    _lock->unlock();
   }
 };
 

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

@@ -241,7 +241,7 @@ void ShenandoahNMethodTable::register_nmethod(nmethod* nm) {
     assert(nm == data->nm(), "Must be same nmethod");
     // Prevent updating a nmethod while concurrent iteration is in progress.
     wait_until_concurrent_iteration_done();
-    ShenandoahReentrantLocker data_locker(data->lock());
+    ShenandoahNMethodLocker data_locker(data->lock());
     data->update();
   } else {
     // For a new nmethod, we can safely append it to the list, because

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

@@ -33,6 +33,10 @@
 #include "runtime/atomic.hpp"
 #include "utilities/growableArray.hpp"
 
+// Use ShenandoahReentrantLock as ShenandoahNMethodLock
+typedef ShenandoahReentrantLock<ShenandoahSimpleLock> ShenandoahNMethodLock;
+typedef ShenandoahLocker<ShenandoahNMethodLock>       ShenandoahNMethodLocker;
+
 // ShenandoahNMethod tuple records the internal locations of oop slots within reclocation stream in
 // the nmethod. This allows us to quickly scan the oops without doing the nmethod-internal scans,
 // that sometimes involves parsing the machine code. Note it does not record the oops themselves,
@@ -44,16 +48,16 @@ private:
   int                     _oops_count;
   bool                    _has_non_immed_oops;
   bool                    _unregistered;
-  ShenandoahReentrantLock _lock;
-  ShenandoahReentrantLock _ic_lock;
+  ShenandoahNMethodLock   _lock;
+  ShenandoahNMethodLock   _ic_lock;
 
 public:
   ShenandoahNMethod(nmethod *nm, GrowableArray<oop*>& oops, bool has_non_immed_oops);
   ~ShenandoahNMethod();
 
   inline nmethod* nm() const;
-  inline ShenandoahReentrantLock* lock();
-  inline ShenandoahReentrantLock* ic_lock();
+  inline ShenandoahNMethodLock* lock();
+  inline ShenandoahNMethodLock* ic_lock();
   inline void oops_do(OopClosure* oops, bool fix_relocations = false);
   // Update oops when the nmethod is re-registered
   void update();
@@ -61,8 +65,8 @@ public:
   inline bool is_unregistered() const;
 
   static ShenandoahNMethod* for_nmethod(nmethod* nm);
-  static inline ShenandoahReentrantLock* lock_for_nmethod(nmethod* nm);
-  static inline ShenandoahReentrantLock* ic_lock_for_nmethod(nmethod* nm);
+  static inline ShenandoahNMethodLock* lock_for_nmethod(nmethod* nm);
+  static inline ShenandoahNMethodLock* ic_lock_for_nmethod(nmethod* nm);
 
   static void heal_nmethod(nmethod* nm);
   static inline void heal_nmethod_metadata(ShenandoahNMethod* nmethod_data);

src/hotspot/share/gc/shenandoah/shenandoahNMethod.inline.hpp

@@ -35,11 +35,11 @@ nmethod* ShenandoahNMethod::nm() const {
   return _nm;
 }
 
-ShenandoahReentrantLock* ShenandoahNMethod::lock() {
+ShenandoahNMethodLock* ShenandoahNMethod::lock() {
   return &_lock;
 }
 
-ShenandoahReentrantLock* ShenandoahNMethod::ic_lock() {
+ShenandoahNMethodLock* ShenandoahNMethod::ic_lock() {
   return &_ic_lock;
 }
 
@@ -85,11 +85,11 @@ void ShenandoahNMethod::attach_gc_data(nmethod* nm, ShenandoahNMethod* gc_data)
   nm->set_gc_data<ShenandoahNMethod>(gc_data);
 }
 
-ShenandoahReentrantLock* ShenandoahNMethod::lock_for_nmethod(nmethod* nm) {
+ShenandoahNMethodLock* ShenandoahNMethod::lock_for_nmethod(nmethod* nm) {
   return gc_data(nm)->lock();
 }
 
-ShenandoahReentrantLock* ShenandoahNMethod::ic_lock_for_nmethod(nmethod* nm) {
+ShenandoahNMethodLock* ShenandoahNMethod::ic_lock_for_nmethod(nmethod* nm) {
   return gc_data(nm)->ic_lock();
 }
 

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

@@ -80,7 +80,7 @@ public:
   virtual bool has_dead_oop(nmethod* nm) const {
     assert(ShenandoahHeap::heap()->is_concurrent_weak_root_in_progress(), "Only for this phase");
     ShenandoahNMethod* data = ShenandoahNMethod::gc_data(nm);
-    ShenandoahReentrantLocker locker(data->lock());
+    ShenandoahNMethodLocker locker(data->lock());
     ShenandoahIsUnloadingOopClosure cl;
     data->oops_do(&cl);
     return  cl.is_unloading();
@@ -90,14 +90,14 @@ public:
 class ShenandoahCompiledICProtectionBehaviour : public CompiledICProtectionBehaviour {
 public:
   virtual bool lock(nmethod* nm) {
-    ShenandoahReentrantLock* const lock = ShenandoahNMethod::ic_lock_for_nmethod(nm);
+    ShenandoahNMethodLock* const lock = ShenandoahNMethod::ic_lock_for_nmethod(nm);
     assert(lock != nullptr, "Not yet registered?");
     lock->lock();
     return true;
   }
 
   virtual void unlock(nmethod* nm) {
-    ShenandoahReentrantLock* const lock = ShenandoahNMethod::ic_lock_for_nmethod(nm);
+    ShenandoahNMethodLock* const lock = ShenandoahNMethod::ic_lock_for_nmethod(nm);
     assert(lock != nullptr, "Not yet registered?");
     lock->unlock();
   }
@@ -107,7 +107,7 @@ public:
       return true;
     }
 
-    ShenandoahReentrantLock* const lock = ShenandoahNMethod::ic_lock_for_nmethod(nm);
+    ShenandoahNMethodLock* const lock = ShenandoahNMethod::ic_lock_for_nmethod(nm);
     assert(lock != nullptr, "Not yet registered?");
     return lock->owned_by_self();
   }