8236880: Shenandoah: Move string dedup cleanup into concurrent phase

Reviewed-by: rkennke, shade

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

@@ -25,6 +25,7 @@
 #define SHARE_GC_SHENANDOAH_SHENANDOAHCLOSURES_HPP
 
 #include "memory/iterator.hpp"
+#include "oops/accessDecorators.hpp"
 
 class ShenandoahHeap;
 class ShenandoahMarkingContext;
@@ -81,6 +82,7 @@ private:
   inline void do_oop_work(T* p);
 };
 
+template <DecoratorSet MO = MO_UNORDERED>
 class ShenandoahEvacuateUpdateRootsClosure: public BasicOopIterateClosure {
 private:
   ShenandoahHeap* _heap;

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

@@ -106,12 +106,14 @@ void ShenandoahTraversalUpdateRefsClosure::do_oop_work(T* p) {
 void ShenandoahTraversalUpdateRefsClosure::do_oop(oop* p)       { do_oop_work(p); }
 void ShenandoahTraversalUpdateRefsClosure::do_oop(narrowOop* p) { do_oop_work(p); }
 
-ShenandoahEvacuateUpdateRootsClosure::ShenandoahEvacuateUpdateRootsClosure() :
+template <DecoratorSet MO>
+ShenandoahEvacuateUpdateRootsClosure<MO>::ShenandoahEvacuateUpdateRootsClosure() :
   _heap(ShenandoahHeap::heap()), _thread(Thread::current()) {
 }
 
+template <DecoratorSet MO>
 template <class T>
-void ShenandoahEvacuateUpdateRootsClosure::do_oop_work(T* p) {
+void ShenandoahEvacuateUpdateRootsClosure<MO>::do_oop_work(T* p) {
   assert(_heap->is_concurrent_root_in_progress(), "Only do this when evacuation is in progress");
 
   T o = RawAccess<>::oop_load(p);
@@ -124,15 +126,17 @@ void ShenandoahEvacuateUpdateRootsClosure::do_oop_work(T* p) {
       if (resolved == obj) {
         resolved = _heap->evacuate_object(obj, _thread);
       }
-      RawAccess<IS_NOT_NULL>::oop_store(p, resolved);
+      RawAccess<IS_NOT_NULL | MO>::oop_store(p, resolved);
     }
   }
 }
-void ShenandoahEvacuateUpdateRootsClosure::do_oop(oop* p) {
+template <DecoratorSet MO>
+void ShenandoahEvacuateUpdateRootsClosure<MO>::do_oop(oop* p) {
   do_oop_work(p);
 }
 
-void ShenandoahEvacuateUpdateRootsClosure::do_oop(narrowOop* p) {
+template <DecoratorSet MO>
+void ShenandoahEvacuateUpdateRootsClosure<MO>::do_oop(narrowOop* p) {
   do_oop_work(p);
 }
 

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

@@ -1064,7 +1064,7 @@ public:
   void work(uint worker_id) {
     ShenandoahParallelWorkerSession worker_session(worker_id);
     ShenandoahEvacOOMScope oom_evac_scope;
-    ShenandoahEvacuateUpdateRootsClosure cl;
+    ShenandoahEvacuateUpdateRootsClosure<> cl;
     MarkingCodeBlobClosure blobsCl(&cl, CodeBlobToOopClosure::FixRelocations);
     _rp->roots_do(worker_id, &cl);
   }
@@ -1306,10 +1306,9 @@ void ShenandoahHeap::object_iterate(ObjectClosure* cl) {
   ShenandoahHeapIterationRootScanner rp;
   ObjectIterateScanRootClosure oops(&_aux_bit_map, &oop_stack);
 
-  // If we are unloading classes right now, we should not touch weak roots,
+  // When concurrent root is in progress, weak roots may contain dead oops, they should not be used
-  // on the off-chance we would evacuate them and make them live accidentally.
+  // for root scanning.
-  // In other cases, we have to scan all roots.
+  if (is_concurrent_root_in_progress()) {
-  if (is_evacuation_in_progress() && unload_classes()) {
     rp.strong_roots_do(&oops);
   } else {
     rp.roots_do(&oops);
@@ -1579,6 +1578,7 @@ void ShenandoahHeap::op_final_mark() {
         if (ShenandoahConcurrentRoots::should_do_concurrent_roots()) {
           types = ShenandoahRootVerifier::combine(ShenandoahRootVerifier::JNIHandleRoots, ShenandoahRootVerifier::WeakRoots);
           types = ShenandoahRootVerifier::combine(types, ShenandoahRootVerifier::CLDGRoots);
+          types = ShenandoahRootVerifier::combine(types, ShenandoahRootVerifier::StringDedupRoots);
         }
 
         if (ShenandoahConcurrentRoots::should_do_concurrent_class_unloading()) {
@@ -1655,6 +1655,7 @@ private:
   ShenandoahVMRoots<true /*concurrent*/>        _vm_roots;
   ShenandoahWeakRoots<true /*concurrent*/>      _weak_roots;
   ShenandoahClassLoaderDataRoots<true /*concurrent*/, false /*single threaded*/> _cld_roots;
+  ShenandoahConcurrentStringDedupRoots          _dedup_roots;
   bool                                          _include_weak_roots;
 
 public:
@@ -1677,10 +1678,16 @@ public:
     }
 
     {
-      ShenandoahEvacuateUpdateRootsClosure cl;
+      ShenandoahEvacuateUpdateRootsClosure<> cl;
       CLDToOopClosure clds(&cl, ClassLoaderData::_claim_strong);
       _cld_roots.cld_do(&clds);
     }
+
+    {
+      ShenandoahForwardedIsAliveClosure is_alive;
+      ShenandoahEvacuateUpdateRootsClosure<MO_RELEASE> keep_alive;
+      _dedup_roots.oops_do(&is_alive, &keep_alive, worker_id);
+    }
   }
 };
 

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

@@ -182,7 +182,7 @@ void ShenandoahNMethod::heal_nmethod(nmethod* nm) {
   assert(data->lock()->owned_by_self(), "Must hold the lock");
 
   ShenandoahEvacOOMScope evac_scope;
-  ShenandoahEvacuateUpdateRootsClosure cl;
+  ShenandoahEvacuateUpdateRootsClosure<> cl;
   data->oops_do(&cl, true /*fix relocation*/);
 }
 

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

@@ -150,6 +150,33 @@ void ShenandoahStringDedupRoots::oops_do(BoolObjectClosure* is_alive, OopClosure
   }
 }
 
+ShenandoahConcurrentStringDedupRoots::ShenandoahConcurrentStringDedupRoots() {
+  if (ShenandoahStringDedup::is_enabled()) {
+    StringDedup::gc_prologue(true);
+    StringDedupTable_lock->lock_without_safepoint_check();
+    StringDedupQueue_lock->lock_without_safepoint_check();
+  }
+}
+
+ShenandoahConcurrentStringDedupRoots::~ShenandoahConcurrentStringDedupRoots() {
+  if (ShenandoahStringDedup::is_enabled()) {
+    StringDedup::gc_epilogue();
+    StringDedupQueue_lock->unlock();
+    StringDedupTable_lock->unlock();
+  }
+}
+
+void ShenandoahConcurrentStringDedupRoots::oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive, uint worker_id) {
+  if (ShenandoahStringDedup::is_enabled()) {
+    assert_locked_or_safepoint_weak(StringDedupQueue_lock);
+    assert_locked_or_safepoint_weak(StringDedupTable_lock);
+
+    StringDedupUnlinkOrOopsDoClosure sd_cl(is_alive, keep_alive);
+    StringDedupQueue::unlink_or_oops_do(&sd_cl);
+    StringDedupTable::unlink_or_oops_do(&sd_cl, worker_id);
+  }
+}
+
 ShenandoahRootProcessor::ShenandoahRootProcessor(ShenandoahPhaseTimings::Phase phase) :
   _heap(ShenandoahHeap::heap()),
   _phase(phase) {
@@ -187,6 +214,7 @@ void ShenandoahRootEvacuator::roots_do(uint worker_id, OopClosure* oops) {
     _vm_roots.oops_do<OopClosure>(oops, worker_id);
     _cld_roots.cld_do(&clds, worker_id);
     _weak_roots.oops_do<OopClosure>(oops, worker_id);
+    _dedup_roots.oops_do(&always_true, oops, worker_id);
   }
 
   if (_include_concurrent_code_roots) {
@@ -195,8 +223,6 @@ void ShenandoahRootEvacuator::roots_do(uint worker_id, OopClosure* oops) {
   } else {
     _thread_roots.oops_do(oops, codes_cl, worker_id);
   }
-
-  _dedup_roots.oops_do(&always_true, oops, worker_id);
 }
 
 ShenandoahRootUpdater::ShenandoahRootUpdater(uint n_workers, ShenandoahPhaseTimings::Phase phase) :

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

@@ -188,6 +188,14 @@ public:
   void oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive, uint worker_id);
 };
 
+class ShenandoahConcurrentStringDedupRoots {
+public:
+  ShenandoahConcurrentStringDedupRoots();
+  ~ShenandoahConcurrentStringDedupRoots();
+
+  void oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive, uint worker_id);
+};
+
 template <typename ITR>
 class ShenandoahCodeCacheRoots {
 private:

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

@@ -32,8 +32,8 @@
 template <uint buffer_size>
 class ShenandoahOopBuffer : public CHeapObj<mtGC> {
 private:
-  oop   _buf[buffer_size];
+  oop           _buf[buffer_size];
-  uint  _index;
+  volatile uint _index;
   ShenandoahOopBuffer<buffer_size>* _next;
 
 public:
@@ -53,6 +53,10 @@ public:
 
   void unlink_or_oops_do(StringDedupUnlinkOrOopsDoClosure* cl);
   void oops_do(OopClosure* cl);
+
+private:
+  uint index_acquire() const;
+  void set_index_release(uint index);
 };
 
 typedef ShenandoahOopBuffer<64> ShenandoahQueueBuffer;

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Red Hat, Inc. All rights reserved.
+ * Copyright (c) 2017, 2020, Red Hat, Inc. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,7 +26,17 @@
 #define SHARE_GC_SHENANDOAH_SHENANDOAHSTRDEDUPQUEUE_INLINE_HPP
 
 #include "gc/shenandoah/shenandoahStrDedupQueue.hpp"
+#include "oops/access.hpp"
+#include "runtime/atomic.hpp"
 
+// With concurrent string dedup cleaning up, GC worker threads
+// may see oops just enqueued, so release_store and load_acquire
+// relationship needs to be established between enqueuing threads
+// and GC workers.
+// For example, when GC sees a slot (index), there must be a valid
+// (dead or live) oop.
+// Note: There is no concern if GC misses newly enqueued oops,
+// since LRB ensures they are in to-space.
 template <uint buffer_size>
 ShenandoahOopBuffer<buffer_size>::ShenandoahOopBuffer() :
   _index(0), _next(NULL) {
@@ -34,29 +44,34 @@ ShenandoahOopBuffer<buffer_size>::ShenandoahOopBuffer() :
 
 template <uint buffer_size>
 bool ShenandoahOopBuffer<buffer_size>::is_full() const {
-  return _index >= buffer_size;
+  return index_acquire() >= buffer_size;
 }
 
 template <uint buffer_size>
 bool ShenandoahOopBuffer<buffer_size>::is_empty() const {
-  return _index == 0;
+  return index_acquire() == 0;
 }
 
 template <uint buffer_size>
 uint ShenandoahOopBuffer<buffer_size>::size() const {
-  return _index;
+  return index_acquire();
 }
 
 template <uint buffer_size>
 void ShenandoahOopBuffer<buffer_size>::push(oop obj) {
   assert(!is_full(),  "Buffer is full");
-  _buf[_index ++] = obj;
+  uint idx = index_acquire();
+  RawAccess<IS_NOT_NULL>::oop_store(&_buf[idx], obj);
+  set_index_release(idx + 1);
 }
 
 template <uint buffer_size>
 oop ShenandoahOopBuffer<buffer_size>::pop() {
   assert(!is_empty(), "Buffer is empty");
-  return _buf[--_index];
+  uint idx = index_acquire() - 1;
+  oop value = NativeAccess<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE | MO_ACQUIRE>::oop_load(&_buf[idx]);
+  set_index_release(idx);
+  return value;
 }
 
 template <uint buffer_size>
@@ -75,15 +90,26 @@ void ShenandoahOopBuffer<buffer_size>::reset() {
   _next = NULL;
 }
 
+template <uint buffer_size>
+uint ShenandoahOopBuffer<buffer_size>::index_acquire() const {
+  return Atomic::load_acquire(&_index);
+}
+
+template <uint buffer_size>
+void ShenandoahOopBuffer<buffer_size>::set_index_release(uint index) {
+  return Atomic::release_store(&_index, index);
+}
+
 template <uint buffer_size>
 void ShenandoahOopBuffer<buffer_size>::unlink_or_oops_do(StringDedupUnlinkOrOopsDoClosure* cl) {
-  for (uint index = 0; index < size(); index ++) {
+  uint len = size();
+  for (uint index = 0; index < len; index ++) {
     oop* obj_addr = &_buf[index];
     if (*obj_addr != NULL) {
       if (cl->is_alive(*obj_addr)) {
         cl->keep_alive(obj_addr);
       } else {
-        *obj_addr = NULL;
+        RawAccess<MO_RELEASE>::oop_store(&_buf[index], oop());
       }
     }
   }
@@ -91,7 +117,8 @@ void ShenandoahOopBuffer<buffer_size>::unlink_or_oops_do(StringDedupUnlinkOrOops
 
 template <uint buffer_size>
 void ShenandoahOopBuffer<buffer_size>::oops_do(OopClosure* cl) {
-  for (uint index = 0; index < size(); index ++) {
+  uint len = size();
+  for (uint index = 0; index < len; index ++) {
     cl->do_oop(&_buf[index]);
   }
 }

src/hotspot/share/runtime/mutexLocker.cpp

@@ -228,7 +228,7 @@ void mutex_init() {
   }
   if (UseShenandoahGC) {
     def(StringDedupQueue_lock      , PaddedMonitor, leaf,        true,  _safepoint_check_never);
-    def(StringDedupTable_lock      , PaddedMutex  , leaf,        true,  _safepoint_check_never);
+    def(StringDedupTable_lock      , PaddedMutex  , leaf + 1,    true,  _safepoint_check_never);
   }
   def(ParGCRareEvent_lock          , PaddedMutex  , leaf     ,   true,  _safepoint_check_always);
   def(CGCPhaseManager_lock         , PaddedMonitor, leaf,        false, _safepoint_check_always);