From: Justin Gibbs <gibbs@fb.com>
Date: Thu, 24 Sep 2015 16:44:07 +0000 (-0700)
Subject: emplace() support for AtomicHashArray/Map
X-Git-Tag: deprecate-dynamic-initializer~377
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=800a6af06f7ad195a8e63d484b2f5cd7c19655fd;p=folly.git

emplace() support for AtomicHashArray/Map

Summary: Allow objects that only support in-place construction (no copy/move
constructor) to be stored in AtomicHashMap and AtomicHashArray via the
emplace() method. This uses variadic template parameters and perfect
forwarding to allow the arguments for any valid constructor for the
object to be used during insertion.

Reviewed By: @nbronson

Differential Revision: D2458152
---

diff --git a/folly/AtomicHashArray-inl.h b/folly/AtomicHashArray-inl.h
index 80272d4a..7d6b13e1 100644
--- a/folly/AtomicHashArray-inl.h
+++ b/folly/AtomicHashArray-inl.h
@@ -83,11 +83,11 @@ findInternal(const KeyT key_in) {
  */
 template <class KeyT, class ValueT,
           class HashFcn, class EqualFcn, class Allocator>
-template <class T>
+template <typename... ArgTs>
 typename AtomicHashArray<KeyT, ValueT,
          HashFcn, EqualFcn, Allocator>::SimpleRetT
 AtomicHashArray<KeyT, ValueT, HashFcn, EqualFcn, Allocator>::
-insertInternal(KeyT key_in, T&& value) {
+insertInternal(KeyT key_in, ArgTs&&... vCtorArgs) {
   const short NO_NEW_INSERTS = 1;
   const short NO_PENDING_INSERTS = 2;
   CHECK_NE(key_in, kEmptyKey_);
@@ -133,12 +133,7 @@ insertInternal(KeyT key_in, T&& value) {
           // Write the value - done before unlocking
           try {
             DCHECK(relaxedLoadKey(*cell) == kLockedKey_);
-            /*
-             * This happens using the copy constructor because we won't have
-             * constructed a lhs to use an assignment operator on when
-             * values are being set.
-             */
-            new (&cell->second) ValueT(std::forward<T>(value));
+            new (&cell->second) ValueT(std::forward<ArgTs>(vCtorArgs)...);
             unlockCell(cell, key_in); // Sets the new key
           } catch (...) {
             // Transition back to empty key---requires handling
diff --git a/folly/AtomicHashArray.h b/folly/AtomicHashArray.h
index 6da92b65..fbe976d2 100644
--- a/folly/AtomicHashArray.h
+++ b/folly/AtomicHashArray.h
@@ -161,11 +161,21 @@ class AtomicHashArray : boost::noncopyable {
    *   iterator is set to the existing entry.
    */
   std::pair<iterator,bool> insert(const value_type& r) {
-    SimpleRetT ret = insertInternal(r.first, r.second);
-    return std::make_pair(iterator(this, ret.idx), ret.success);
+    return emplace(r.first, r.second);
   }
   std::pair<iterator,bool> insert(value_type&& r) {
-    SimpleRetT ret = insertInternal(r.first, std::move(r.second));
+    return emplace(r.first, std::move(r.second));
+  }
+
+  /*
+   * emplace --
+   *
+   *   Same contract as insert(), but performs in-place construction
+   *   of the value type using the specified arguments.
+   */
+  template <typename... ArgTs>
+  std::pair<iterator,bool> emplace(KeyT key_in, ArgTs&&... vCtorArgs) {
+    SimpleRetT ret = insertInternal(key_in, std::forward<ArgTs>(vCtorArgs)...);
     return std::make_pair(iterator(this, ret.idx), ret.success);
   }
 
@@ -237,8 +247,8 @@ class AtomicHashArray : boost::noncopyable {
     SimpleRetT() = default;
   };
 
-  template <class T>
-  SimpleRetT insertInternal(KeyT key, T&& value);
+  template <typename... ArgTs>
+  SimpleRetT insertInternal(KeyT key, ArgTs&&... vCtorArgs);
 
   SimpleRetT findInternal(const KeyT key);
 
diff --git a/folly/AtomicHashMap-inl.h b/folly/AtomicHashMap-inl.h
index 92c0a150..6d8bb917 100644
--- a/folly/AtomicHashMap-inl.h
+++ b/folly/AtomicHashMap-inl.h
@@ -40,26 +40,15 @@ AtomicHashMap(size_t finalSizeEst, const Config& config)
   numMapsAllocated_.store(1, std::memory_order_relaxed);
 }
 
-// insert --
+// emplace --
 template <typename KeyT, typename ValueT,
           typename HashFcn, typename EqualFcn, typename Allocator>
+template <typename... ArgTs>
 std::pair<typename AtomicHashMap<KeyT, ValueT, HashFcn,
                                  EqualFcn, Allocator>::iterator, bool>
 AtomicHashMap<KeyT, ValueT, HashFcn, EqualFcn, Allocator>::
-insert(key_type k, const mapped_type& v) {
-  SimpleRetT ret = insertInternal(k,v);
-  SubMap* subMap = subMaps_[ret.i].load(std::memory_order_relaxed);
-  return std::make_pair(iterator(this, ret.i, subMap->makeIter(ret.j)),
-                        ret.success);
-}
-
-template <typename KeyT, typename ValueT,
-          typename HashFcn, typename EqualFcn, typename Allocator>
-std::pair<typename AtomicHashMap<KeyT, ValueT, HashFcn,
-                                 EqualFcn, Allocator>::iterator, bool>
-AtomicHashMap<KeyT, ValueT, HashFcn, EqualFcn, Allocator>::
-insert(key_type k, mapped_type&& v) {
-  SimpleRetT ret = insertInternal(k, std::move(v));
+emplace(key_type k, ArgTs&&... vCtorArgs) {
+  SimpleRetT ret = insertInternal(k, std::forward<ArgTs>(vCtorArgs)...);
   SubMap* subMap = subMaps_[ret.i].load(std::memory_order_relaxed);
   return std::make_pair(iterator(this, ret.i, subMap->makeIter(ret.j)),
                         ret.success);
@@ -68,10 +57,10 @@ insert(key_type k, mapped_type&& v) {
 // insertInternal -- Allocates new sub maps as existing ones fill up.
 template <typename KeyT, typename ValueT,
           typename HashFcn, typename EqualFcn, typename Allocator>
-template <class T>
+template <typename... ArgTs>
 typename AtomicHashMap<KeyT, ValueT, HashFcn, EqualFcn, Allocator>::SimpleRetT
 AtomicHashMap<KeyT, ValueT, HashFcn, EqualFcn, Allocator>::
-insertInternal(key_type key, T&& value) {
+insertInternal(key_type key, ArgTs&&... vCtorArgs) {
  beginInsertInternal:
   auto nextMapIdx = // this maintains our state
     numMapsAllocated_.load(std::memory_order_acquire);
@@ -79,7 +68,7 @@ insertInternal(key_type key, T&& value) {
   FOR_EACH_RANGE(i, 0, nextMapIdx) {
     // insert in each map successively.  If one succeeds, we're done!
     SubMap* subMap = subMaps_[i].load(std::memory_order_relaxed);
-    ret = subMap->insertInternal(key, std::forward<T>(value));
+    ret = subMap->insertInternal(key, std::forward<ArgTs>(vCtorArgs)...);
     if (ret.idx == subMap->capacity_) {
       continue;  //map is full, so try the next one
     }
@@ -134,7 +123,7 @@ insertInternal(key_type key, T&& value) {
   // just did a spin wait with an acquire load on numMapsAllocated_.
   SubMap* loadedMap = subMaps_[nextMapIdx].load(std::memory_order_relaxed);
   DCHECK(loadedMap && loadedMap != (SubMap*)kLockedPtr_);
-  ret = loadedMap->insertInternal(key, std::forward<T>(value));
+  ret = loadedMap->insertInternal(key, std::forward<ArgTs>(vCtorArgs)...);
   if (ret.idx != loadedMap->capacity_) {
     return SimpleRetT(nextMapIdx, ret.idx, ret.success);
   }
diff --git a/folly/AtomicHashMap.h b/folly/AtomicHashMap.h
index fb353887..90c59661 100644
--- a/folly/AtomicHashMap.h
+++ b/folly/AtomicHashMap.h
@@ -205,8 +205,6 @@ class AtomicHashMap : boost::noncopyable {
   key_equal key_eq() const { return key_equal(); }
   hasher hash_function() const { return hasher(); }
 
-  // TODO: emplace() support would be nice.
-
   /*
    * insert --
    *
@@ -223,13 +221,26 @@ class AtomicHashMap : boost::noncopyable {
    *   AtomicHashMapFullError is thrown.
    */
   std::pair<iterator,bool> insert(const value_type& r) {
-    return insert(r.first, r.second);
+    return emplace(r.first, r.second);
+  }
+  std::pair<iterator,bool> insert(key_type k, const mapped_type& v) {
+    return emplace(k, v);
   }
-  std::pair<iterator,bool> insert(key_type k, const mapped_type& v);
   std::pair<iterator,bool> insert(value_type&& r) {
-    return insert(r.first, std::move(r.second));
+    return emplace(r.first, std::move(r.second));
   }
-  std::pair<iterator,bool> insert(key_type k, mapped_type&& v);
+  std::pair<iterator,bool> insert(key_type k, mapped_type&& v) {
+    return emplace(k, std::move(v));
+  }
+
+  /*
+   * emplace --
+   *
+   *   Same contract as insert(), but performs in-place construction
+   *   of the value type using the specified arguments.
+   */
+  template <typename... ArgTs>
+  std::pair<iterator,bool> emplace(key_type k, ArgTs&&... vCtorArg);
 
   /*
    * find --
@@ -391,8 +402,8 @@ class AtomicHashMap : boost::noncopyable {
     SimpleRetT() = default;
   };
 
-  template <class T>
-  SimpleRetT insertInternal(KeyT key, T&& value);
+  template <typename... ArgTs>
+  SimpleRetT insertInternal(KeyT key, ArgTs&&... value);
 
   SimpleRetT findInternal(const KeyT k) const;
 
diff --git a/folly/test/AtomicHashArrayTest.cpp b/folly/test/AtomicHashArrayTest.cpp
index c1eaf6d9..3c7c32f2 100644
--- a/folly/test/AtomicHashArrayTest.cpp
+++ b/folly/test/AtomicHashArrayTest.cpp
@@ -148,11 +148,17 @@ template<class KeyT, class ValueT, class Allocator = std::allocator<char>>
 void testNoncopyableMap() {
   typedef AtomicHashArray<KeyT, std::unique_ptr<ValueT>, std::hash<KeyT>,
                           std::equal_to<KeyT>, Allocator> MyArr;
-  auto arr = MyArr::create(150);
+  auto arr = MyArr::create(250);
   for (int i = 0; i < 100; i++) {
     arr->insert(make_pair(i,std::unique_ptr<ValueT>(new ValueT(i))));
   }
-  for (int i = 0; i < 100; i++) {
+  for (int i = 100; i < 150; i++) {
+    arr->emplace(i,new ValueT(i));
+  }
+  for (int i = 150; i < 200; i++) {
+    arr->emplace(i,new ValueT(i),std::default_delete<ValueT>());
+  }
+  for (int i = 0; i < 200; i++) {
     auto ret = arr->find(i);
     EXPECT_EQ(*(ret->second), i);
   }
diff --git a/folly/test/AtomicHashMapTest.cpp b/folly/test/AtomicHashMapTest.cpp
index a613a7c2..0a65d361 100644
--- a/folly/test/AtomicHashMapTest.cpp
+++ b/folly/test/AtomicHashMapTest.cpp
@@ -78,13 +78,19 @@ TEST(Ahm, BasicNoncopyable) {
   for (int i = 50; i < 100; ++i) {
     myMap.insert(i, std::unique_ptr<int>(new int (i)));
   }
-  for (int i = 0; i < 100; ++i) {
+  for (int i = 100; i < 150; ++i) {
+    myMap.emplace(i, new int (i));
+  }
+  for (int i = 150; i < 200; ++i) {
+    myMap.emplace(i, new int (i), std::default_delete<int>());
+  }
+  for (int i = 0; i < 200; ++i) {
     EXPECT_EQ(*(myMap.find(i)->second), i);
   }
-  for (int i = 0; i < 100; i+=4) {
+  for (int i = 0; i < 200; i+=4) {
     myMap.erase(i);
   }
-  for (int i = 0; i < 100; i+=4) {
+  for (int i = 0; i < 200; i+=4) {
     EXPECT_EQ(myMap.find(i), myMap.end());
   }
 }