Make AtomicHashMap support move constructible types

Summary: modified AtomicHashArray and AtomicHashMap to support move constructible types

Test Plan: tested with fbcode/folly/test/AtomicHashArrayTest.cpp and fbcode/folly/test/AtomicHashMapTest.cpp

Reviewed By: philipp@fb.com

FB internal diff: D527270

folly/AtomicHashArray-inl.h

@@ -78,12 +78,12 @@ findInternal(const KeyT key_in) {
  *   default.
  */
 template <class KeyT, class ValueT, class HashFcn>
+template <class T>
 typename AtomicHashArray<KeyT, ValueT, HashFcn>::SimpleRetT
 AtomicHashArray<KeyT, ValueT, HashFcn>::
-insertInternal(const value_type& record) {
+insertInternal(KeyT key_in, T&& value) {
   const short NO_NEW_INSERTS = 1;
   const short NO_PENDING_INSERTS = 2;
-  const KeyT key_in = record.first;
   CHECK_NE(key_in, kEmptyKey_);
   CHECK_NE(key_in, kLockedKey_);
   CHECK_NE(key_in, kErasedKey_);
@@ -131,7 +131,7 @@ insertInternal(const value_type& record) {
              * constructed a lhs to use an assignment operator on when
              * values are being set.
              */
-            new (&cell->second) ValueT(record.second);
+            new (&cell->second) ValueT(std::forward<T>(value));
             unlockCell(cell, key_in); // Sets the new key
           } catch (...) {
             // Transition back to empty key---requires handling

folly/AtomicHashArray.h

@@ -150,7 +150,11 @@ class AtomicHashArray : boost::noncopyable {
    *   iterator is set to the existing entry.
    */
   std::pair<iterator,bool> insert(const value_type& r) {
-    SimpleRetT ret = insertInternal(r);
+    SimpleRetT ret = insertInternal(r.first, r.second);
+    return std::make_pair(iterator(this, ret.idx), ret.success);
+  }
+  std::pair<iterator,bool> insert(value_type&& r) {
+    SimpleRetT ret = insertInternal(r.first, std::move(r.second));
     return std::make_pair(iterator(this, ret.idx), ret.success);
   }
 
@@ -213,7 +217,8 @@ class AtomicHashArray : boost::noncopyable {
     SimpleRetT() {}
   };
 
-  SimpleRetT insertInternal(const value_type& record);
+  template <class T>
+  SimpleRetT insertInternal(KeyT key, T&& value);
 
   SimpleRetT findInternal(const KeyT key);
 

folly/AtomicHashMap-inl.h

@@ -46,8 +46,18 @@ AtomicHashMap(size_t size, const Config& config)
 template <typename KeyT, typename ValueT, typename HashFcn>
 std::pair<typename AtomicHashMap<KeyT,ValueT,HashFcn>::iterator,bool>
 AtomicHashMap<KeyT, ValueT, HashFcn>::
-insert(const value_type& r) {
-  SimpleRetT ret = insertInternal(r);
+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>
+std::pair<typename AtomicHashMap<KeyT,ValueT,HashFcn>::iterator,bool>
+AtomicHashMap<KeyT, ValueT, HashFcn>::
+insert(key_type k, mapped_type&& v) {
+  SimpleRetT ret = insertInternal(k, std::move(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);
@@ -55,9 +65,10 @@ insert(const value_type& r) {
 
 // insertInternal -- Allocates new sub maps as existing ones fill up.
 template <typename KeyT, typename ValueT, typename HashFcn>
+template <class T>
 typename AtomicHashMap<KeyT, ValueT, HashFcn>::SimpleRetT
 AtomicHashMap<KeyT, ValueT, HashFcn>::
-insertInternal(const value_type& r) {
+insertInternal(key_type key, T&& value) {
  beginInsertInternal:
   int nextMapIdx = // this maintains our state
     numMapsAllocated_.load(std::memory_order_acquire);
@@ -66,7 +77,7 @@ insertInternal(const value_type& r) {
   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(r);
+    ret = subMap->insertInternal(key, std::forward<T>(value));
     if (ret.idx == subMap->capacity_) {
       continue;  //map is full, so try the next one
     }
@@ -121,7 +132,7 @@ insertInternal(const value_type& r) {
   // 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(r);
+  ret = loadedMap->insertInternal(key, std::forward<T>(value));
   if (ret.idx != loadedMap->capacity_) {
     return SimpleRetT(nextMapIdx, ret.idx, ret.success);
   }

folly/AtomicHashMap.h

@@ -225,10 +225,14 @@ class AtomicHashMap : boost::noncopyable {
    *   all sub maps are full, no element is inserted, and
    *   AtomicHashMapFullError is thrown.
    */
-  std::pair<iterator,bool> insert(const value_type& r);
-  std::pair<iterator,bool> insert(key_type k, const mapped_type& v) {
-    return insert(value_type(k, v));
+  std::pair<iterator,bool> insert(const value_type& r) {
+    return insert(r.first, r.second);
   }
+  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));
+  }
+  std::pair<iterator,bool> insert(key_type k, mapped_type&& v);
 
   /*
    * find --
@@ -336,7 +340,26 @@ class AtomicHashMap : boost::noncopyable {
   /* Advanced functions for direct access: */
 
   inline uint32_t recToIdx(const value_type& r, bool mayInsert = true) {
-    SimpleRetT ret = mayInsert ? insertInternal(r) : findInternal(r.first);
+    SimpleRetT ret = mayInsert ?
+      insertInternal(r.first, r.second) : findInternal(r.first);
+    return encodeIndex(ret.i, ret.j);
+  }
+
+  inline uint32_t recToIdx(value_type&& r, bool mayInsert = true) {
+    SimpleRetT ret = mayInsert ?
+      insertInternal(r.first, std::move(r.second)) : findInternal(r.first);
+    return encodeIndex(ret.i, ret.j);
+  }
+
+  inline uint32_t recToIdx(key_type k, const mapped_type& v,
+    bool mayInsert = true) {
+    SimpleRetT ret = mayInsert ? insertInternal(k, v) : findInternal(k);
+    return encodeIndex(ret.i, ret.j);
+  }
+
+  inline uint32_t recToIdx(key_type k, mapped_type&& v, bool mayInsert = true) {
+    SimpleRetT ret = mayInsert ?
+      insertInternal(k, std::move(v)) : findInternal(k);
     return encodeIndex(ret.i, ret.j);
   }
 
@@ -367,7 +390,8 @@ class AtomicHashMap : boost::noncopyable {
     SimpleRetT() {}
   };
 
-  SimpleRetT insertInternal(const value_type& r);
+  template <class T>
+  SimpleRetT insertInternal(KeyT key, T&& value);
 
   SimpleRetT findInternal(const KeyT k) const;
 

folly/test/AtomicHashArrayTest.cpp

@@ -75,17 +75,35 @@ void testMap() {
   }
 }
 
+template<class KeyT, class ValueT>
+void testNoncopyableMap() {
+  typedef AtomicHashArray<KeyT, std::unique_ptr<ValueT>>  MyArr;
+  auto arr = MyArr::create(150);
+  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++) {
+    auto ret = arr->find(i);
+    EXPECT_EQ(*(ret->second), i);
+  }
+}
+
+
 TEST(Aha, InsertErase_i32_i32) {
   testMap<int32_t,int32_t>();
+  testNoncopyableMap<int32_t,int32_t>();
 }
 TEST(Aha, InsertErase_i64_i32) {
   testMap<int64_t,int32_t>();
+  testNoncopyableMap<int64_t,int32_t>();
 }
 TEST(Aha, InsertErase_i64_i64) {
   testMap<int64_t,int64_t>();
+  testNoncopyableMap<int64_t,int64_t>();
 }
 TEST(Aha, InsertErase_i32_i64) {
   testMap<int32_t,int64_t>();
+  testNoncopyableMap<int32_t,int64_t>();
 }
 TEST(Aha, InsertErase_i32_str) {
   testMap<int32_t,string>();

folly/test/AtomicHashMapTest.cpp

@@ -21,7 +21,7 @@
 #include <sys/time.h>
 #include <thread>
 #include <atomic>
-
+#include <memory>
 #include "folly/Benchmark.h"
 #include "folly/Conv.h"
 
@@ -66,6 +66,29 @@ TEST(Ahm, BasicStrings) {
   EXPECT_EQ(myMap.find(999)->first, 999);
 }
 
+
+TEST(Ahm, BasicNoncopyable) {
+  typedef AtomicHashMap<int64_t,std::unique_ptr<int>> AHM;
+  AHM myMap(1024);
+  EXPECT_TRUE(myMap.begin() == myMap.end());
+
+  for (int i = 0; i < 50; ++i) {
+    myMap.insert(make_pair(i, std::unique_ptr<int>(new int(i))));
+  }
+  for (int i = 50; i < 100; ++i) {
+    myMap.insert(i, std::unique_ptr<int>(new int (i)));
+  }
+  for (int i = 0; i < 100; ++i) {
+    EXPECT_EQ(*(myMap.find(i)->second), i);
+  }
+  for (int i = 0; i < 100; i+=4) {
+    myMap.erase(i);
+  }
+  for (int i = 0; i < 100; i+=4) {
+    EXPECT_EQ(myMap.find(i), myMap.end());
+  }
+}
+
 typedef int32_t     KeyT;
 typedef int64_t     KeyTBig;
 typedef int32_t     ValueT;
@@ -168,9 +191,9 @@ TEST(Ahm, iterator) {
 
 class Counters {
 private:
-  // NOTE: Unfortunately can't currently put a std::atomic<int64_t> in
-  // the value in ahm since it doesn't support non-copyable but
-  // move-constructible value types yet.
+  // Note: Unfortunately can't currently put a std::atomic<int64_t> in
+  // the value in ahm since it doesn't support types that are both non-copy
+  // and non-move constructible yet.
   AtomicHashMap<int64_t,int64_t> ahm;
 
 public: