Back out "Fix concurrency issues in ConcurrentSkipList."

Summary: This is causing a memory leak

Reviewed By: yfeldblum

Differential Revision: D30082875

fbshipit-source-id: 6b6ba9963cc45d71cb98b88b1db9a304cb7b7bda

folly/ConcurrentSkipList-inl.h

@@ -97,7 +97,7 @@ class SkipListNode {
 
   inline SkipListNode* skip(int layer) const {
     DCHECK_LT(layer, height_);
-    return skip_[layer].load(std::memory_order_acquire);
+    return skip_[layer].load(std::memory_order_consume);
   }
 
   // next valid node as in the linked list
@@ -155,7 +155,7 @@ class SkipListNode {
     }
   }
 
-  uint16_t getFlags() const { return flags_.load(std::memory_order_acquire); }
+  uint16_t getFlags() const { return flags_.load(std::memory_order_consume); }
   void setFlags(uint16_t flags) {
     flags_.store(flags, std::memory_order_release);
   }
@@ -269,41 +269,43 @@ class NodeRecycler<
     dirty_.store(true, std::memory_order_relaxed);
   }
 
-  int addRef() { return refs_.fetch_add(1, std::memory_order_acq_rel); }
+  int addRef() { return refs_.fetch_add(1, std::memory_order_relaxed); }
 
   int releaseRef() {
-    // This if statement is purely an optimization. It's possible that this
-    // misses an opportunity to delete, but that's OK, we'll try again at
-    // the next opportunity. It does not harm the thread safety. For this
-    // reason, we can use relaxed loads to make the decision.
-    if (!dirty_.load(std::memory_order_relaxed) || refs() > 1) {
-      return refs_.fetch_add(-1, std::memory_order_acq_rel);
+    // We don't expect to clean the recycler immediately everytime it is OK
+    // to do so. Here, it is possible that multiple accessors all release at
+    // the same time but nobody would clean the recycler here. If this
+    // happens, the recycler will usually still get cleaned when
+    // such a race doesn't happen. The worst case is the recycler will
+    // eventually get deleted along with the skiplist.
+    if (LIKELY(!dirty_.load(std::memory_order_relaxed) || refs() > 1)) {
+      return refs_.fetch_add(-1, std::memory_order_relaxed);
     }
 
     std::unique_ptr<std::vector<NodeType*>> newNodes;
-    int ret;
     {
-      // The order at which we lock, add, swap, is very important for
-      // correctness.
       std::lock_guard<MicroSpinLock> g(lock_);
-      ret = refs_.fetch_add(-1, std::memory_order_acq_rel);
-      if (ret == 0) {
-        // When refs_ reachs 0, it is safe to remove all the current nodes
-        // in the recycler, as we already acquired the lock here so no more
-        // new nodes can be added, even though new accessors may be added
-        // after this.
-        newNodes.swap(nodes_);
-        dirty_.store(false, std::memory_order_relaxed);
+      if (nodes_.get() == nullptr || refs() > 1) {
+        return refs_.fetch_add(-1, std::memory_order_relaxed);
       }
+      // once refs_ reaches 1 and there is no other accessor, it is safe to
+      // remove all the current nodes in the recycler, as we already acquired
+      // the lock here so no more new nodes can be added, even though new
+      // accessors may be added after that.
+      newNodes.swap(nodes_);
+      dirty_.store(false, std::memory_order_relaxed);
     }
+
     // TODO(xliu) should we spawn a thread to do this when there are large
     // number of nodes in the recycler?
-    if (newNodes) {
-      for (auto& node : *newNodes) {
-        NodeType::destroy(alloc_, node);
-      }
+    for (auto& node : *newNodes) {
+      NodeType::destroy(alloc_, node);
     }
-    return ret;
+
+    // decrease the ref count at the very end, to minimize the
+    // chance of other threads acquiring lock_ to clear the deleted
+    // nodes again.
+    return refs_.fetch_add(-1, std::memory_order_relaxed);
   }
 
   NodeAlloc& alloc() { return alloc_; }

folly/ConcurrentSkipList.h

@@ -250,7 +250,7 @@ class ConcurrentSkipList {
     return foundLayer;
   }
 
-  int height() const { return head_.load(std::memory_order_acquire)->height(); }
+  int height() const { return head_.load(std::memory_order_consume)->height(); }
 
   int maxLayer() const { return height() - 1; }
 
@@ -401,12 +401,12 @@ class ConcurrentSkipList {
   }
 
   const value_type* first() const {
-    auto node = head_.load(std::memory_order_acquire)->skip(0);
+    auto node = head_.load(std::memory_order_consume)->skip(0);
     return node ? &node->data() : nullptr;
   }
 
   const value_type* last() const {
-    NodeType* pred = head_.load(std::memory_order_acquire);
+    NodeType* pred = head_.load(std::memory_order_consume);
     NodeType* node = nullptr;
     for (int layer = maxLayer(); layer >= 0; --layer) {
       do {
@@ -434,7 +434,7 @@ class ConcurrentSkipList {
       int* max_layer) const {
     *max_layer = maxLayer();
     return findInsertionPoint(
-        head_.load(std::memory_order_acquire), *max_layer, data, preds, succs);
+        head_.load(std::memory_order_consume), *max_layer, data, preds, succs);
   }
 
   // Find node for access. Returns a paired values:
@@ -450,7 +450,7 @@ class ConcurrentSkipList {
   // results, this is slightly faster than findNodeRightDown for better
   // localality on the skipping pointers.
   std::pair<NodeType*, int> findNodeDownRight(const value_type& data) const {
-    NodeType* pred = head_.load(std::memory_order_acquire);
+    NodeType* pred = head_.load(std::memory_order_consume);
     int ht = pred->height();
     NodeType* node = nullptr;
 
@@ -478,7 +478,7 @@ class ConcurrentSkipList {
   // find node by first stepping right then stepping down.
   // We still keep this for reference purposes.
   std::pair<NodeType*, int> findNodeRightDown(const value_type& data) const {
-    NodeType* pred = head_.load(std::memory_order_acquire);
+    NodeType* pred = head_.load(std::memory_order_consume);
     NodeType* node = nullptr;
     auto top = maxLayer();
     int found = 0;
@@ -502,7 +502,7 @@ class ConcurrentSkipList {
   }
 
   void growHeight(int height) {
-    NodeType* oldHead = head_.load(std::memory_order_acquire);
+    NodeType* oldHead = head_.load(std::memory_order_consume);
     if (oldHead->height() >= height) { // someone else already did this
       return;
     }
@@ -598,7 +598,7 @@ class ConcurrentSkipList<T, Comp, NodeAlloc, MAX_HEIGHT>::Accessor {
   size_type count(const key_type& data) const { return contains(data); }
 
   iterator begin() const {
-    NodeType* head = sl_->head_.load(std::memory_order_acquire);
+    NodeType* head = sl_->head_.load(std::memory_order_consume);
     return iterator(head->next());
   }
   iterator end() const { return iterator(nullptr); }
@@ -814,7 +814,7 @@ class ConcurrentSkipList<T, Comp, NodeAlloc, MAX_HEIGHT>::Skipper {
 
  private:
   NodeType* head() const {
-    return accessor_.skiplist()->head_.load(std::memory_order_acquire);
+    return accessor_.skiplist()->head_.load(std::memory_order_consume);
   }
 
   Accessor accessor_;