utils/cuckoo_map: add Emplace, move-based Insert

core/utils/cuckoo_map.h

@@ -76,6 +76,7 @@ template <typename K, typename V, typename H = std::hash<K>,
 class CuckooMap {
  public:
   typedef std::pair<K, V> Entry;
+
   class iterator {
    public:
     using difference_type = std::ptrdiff_t;
@@ -173,26 +174,24 @@ class CuckooMap {
   iterator begin() { return iterator(*this, 0, 0); }
   iterator end() { return iterator(*this, buckets_.size(), 0); }
 
-  // Insert/update a key value pair
-  // Return the pointer to the inserted entry
-  Entry* Insert(const K& key, const V& value, const H& hasher = H(),
-                const E& eq = E()) {
+  template <typename... Args>
+  Entry* DoEmplace(const K& key, const H& hasher, const E& eq, Args&&... args) {
     Entry* entry;
     HashResult primary = Hash(key, hasher);
 
     EntryIndex idx = FindWithHash(primary, key, eq);
     if (idx != kInvalidEntryIdx) {
       entry = &entries_[idx];
-      entry->second = value;
+      new (&entry->second) V(std::forward<Args>(args)...);
       return entry;
     }
 
     HashResult secondary = HashSecondary(primary);
 
     int trials = 0;
 
-    while ((entry = AddEntry(primary, secondary, key, value, hasher)) ==
-           nullptr) {
+    while ((entry = EmplaceEntry(primary, secondary, key, hasher,
+                                 std::forward<Args>(args)...)) == nullptr) {
       if (++trials >= 3) {
         LOG_FIRST_N(WARNING, 1)
             << "CuckooMap: Excessive hash colision detected:\n"
@@ -201,11 +200,31 @@ class CuckooMap {
       }
 
       // expand the table as the last resort
-      ExpandBuckets(hasher, eq);
+      ExpandBuckets<std::conditional_t<std::is_move_constructible<V>::value,
+                                       V&&, const V&>>(hasher, eq);
     }
     return entry;
   }
 
+  // Insert/update a key value pair
+  // On success returns a pointer to the inserted entry, nullptr otherwise.
+  // NOTE: when Insert() returns nullptr, the copy/move constructor of `V` may
+  // not be called.
+  template <typename VV>
+  Entry* Insert(const K& key, VV&& value, const H& hasher = H(),
+                const E& eq = E()) {
+    return DoEmplace(key, hasher, eq, std::forward<VV>(value));
+  }
+
+  // Emplace/update-in-place a key value pair
+  // On success returns a pointer to the inserted entry, nullptr otherwise.
+  // NOTE: when Emplace() returns nullptr, the constructor of `V` may not be
+  // called.
+  template <typename... Args>
+  Entry* Emplace(const K& key, Args&&... args) {
+    return DoEmplace(key, H(), E(), std::forward<Args>(args)...);
+  }
+
   // Find the pointer to the stored value by the key.
   // Return nullptr if not exist.
   Entry* Find(const K& key, const H& hasher = H(), const E& eq = E()) {
@@ -303,8 +322,9 @@ class CuckooMap {
 
   // Try to add (key, value) to the bucket indexed by bucket_idx
   // Return the pointer to the entry if success. Otherwise return nullptr.
-  Entry* AddToBucket(HashResult bucket_idx, const K& key, const V& value,
-                     const H& hasher) {
+  template <typename... Args>
+  Entry* EmplaceInBucket(HashResult bucket_idx, const K& key, const H& hasher,
+                         Args&&... args) {
     Bucket& bucket = buckets_[bucket_idx];
     int slot_idx = FindEmptySlot(bucket);
     if (slot_idx == -1) {
@@ -318,7 +338,7 @@ class CuckooMap {
 
     Entry& entry = entries_[free_idx];
     entry.first = key;
-    entry.second = value;
+    new (&entry.second) V(std::forward<Args>(args)...);
 
     num_entries_++;
     return &entry;
@@ -364,20 +384,21 @@ class CuckooMap {
 
   // Try to add the entry (key, value)
   // Return the pointer to the entry if success. Otherwise return nullptr.
-  Entry* AddEntry(HashResult primary, HashResult secondary, const K& key,
-                  const V& value, const H& hasher) {
+  template <typename... Args>
+  Entry* EmplaceEntry(HashResult primary, HashResult secondary, const K& key,
+                      const H& hasher, Args&&... args) {
     HashResult primary_bucket_index, secondary_bucket_index;
     Entry* entry = nullptr;
   again:
     primary_bucket_index = primary & bucket_mask_;
-    if ((entry = AddToBucket(primary_bucket_index, key, value, hasher)) !=
-        nullptr) {
+    if ((entry = EmplaceInBucket(primary_bucket_index, key, hasher,
+                                 std::forward<Args>(args)...)) != nullptr) {
       return entry;
     }
 
     secondary_bucket_index = secondary & bucket_mask_;
-    if ((entry = AddToBucket(secondary_bucket_index, key, value, hasher)) !=
-        nullptr) {
+    if ((entry = EmplaceInBucket(secondary_bucket_index, key, hasher,
+                                 std::forward<Args>(args)...)) != nullptr) {
       return entry;
     }
 
@@ -501,12 +522,14 @@ class CuckooMap {
   }
 
   // Resize the space of buckets, and rehash existing entries
+  template <typename VV>
   void ExpandBuckets(const H& hasher, const E& eq) {
     CuckooMap<K, V, H, E> bigger(buckets_.size() * 2, entries_.size());
 
-    for (const auto& e : *this) {
-      // While very unlikely, this insert() may cause recursive expansion
-      bool ret = bigger.Insert(e.first, e.second, hasher, eq);
+    for (auto& e : *this) {
+      // While very unlikely, this DoEmplace() may cause recursive expansion
+      Entry* ret =
+          bigger.DoEmplace(e.first, hasher, eq, std::forward<VV>(e.second));
       if (!ret) {
         return;
       }

core/utils/cuckoo_map_test.cc

@@ -33,6 +33,52 @@
 
 #include "random.h"
 
+struct CopyConstructorOnly {
+  // FIXME: CuckooMap should work without this default constructor
+  CopyConstructorOnly() = default;
+  CopyConstructorOnly(CopyConstructorOnly &&other) = delete;
+
+  CopyConstructorOnly(int aa, int bb): a(aa), b(bb) {}
+  CopyConstructorOnly(const CopyConstructorOnly &other)
+      : a(other.a), b(other.b) {}
+
+  int a;
+  int b;
+};
+
+struct MoveConstructorOnly {
+  // FIXME: CuckooMap should work without this default constructor
+  MoveConstructorOnly() = default;
+  MoveConstructorOnly(const MoveConstructorOnly &other) = delete;
+
+  MoveConstructorOnly(int aa, int bb): a(aa), b(bb) {}
+  MoveConstructorOnly(MoveConstructorOnly &&other) noexcept
+      : a(other.a), b(other.b) {
+    other.a = 0;
+    other.b = 0;
+  }
+
+  int a;
+  int b;
+};
+
+// C++ has no clean way to specialize templates for derived typess...
+// so we just define a hash functor for each.
+
+template <>
+struct std::hash<CopyConstructorOnly> {
+  std::size_t operator()(const CopyConstructorOnly &t) const noexcept {
+    return std::hash<int>()(t.a + t.b);  // doesn't need to be a good one...
+  }
+};
+
+template <>
+struct std::hash<MoveConstructorOnly> {
+  std::size_t operator()(const MoveConstructorOnly &t) const noexcept {
+    return std::hash<int>()(t.a * t.b);  // doesn't need to be a good one...
+  }
+};
+
 namespace {
 
 using bess::utils::CuckooMap;
@@ -45,6 +91,63 @@ TEST(CuckooMapTest, Insert) {
   EXPECT_EQ(cuckoo.Insert(1, 1)->second, 1);
 }
 
+template<typename T>
+void CompileTimeInstantiation() {
+  std::map<int, T> m1;
+  std::map<T, int> m2;
+  std::map<T, T> m3;
+  std::unordered_map<int, T> u1;
+  std::unordered_map<T, int> u2;
+  std::unordered_map<T, T> u3;
+  std::vector<T> v1;
+
+  // FIXME: currently, CuckooMap does not support types without a default
+  // constructor. The following will fail with the current code.
+  // CuckooMap<int, T> c1;
+  // CuckooMap<T, int> c2;
+  // CuckooMap<T, T> c3;
+}
+
+TEST(CuckooMap, TypeSupport) {
+  // Standard containers, such as std::map and std::vector, should be able to
+  // contain types with various constructor and assignment restrictions.
+  // The below will check this ability at compile time.
+  CompileTimeInstantiation<CopyConstructorOnly>();
+  CompileTimeInstantiation<MoveConstructorOnly>();
+}
+
+// Test insertion with copy
+TEST(CuckooMapTest, CopyInsert) {
+  CuckooMap<uint32_t, CopyConstructorOnly> cuckoo;
+  auto expected = CopyConstructorOnly(1, 2);
+  auto *entry = cuckoo.Insert(10, expected);
+  ASSERT_NE(nullptr, entry);
+  const auto &x = entry->second;
+  EXPECT_EQ(1, x.a);
+  EXPECT_EQ(2, x.b);
+}
+
+// Test insertion with move
+TEST(CuckooMapTest, MoveInsert) {
+  CuckooMap<uint32_t, MoveConstructorOnly> cuckoo;
+  auto expected = MoveConstructorOnly(3, 4);
+  auto *entry = cuckoo.Insert(11, std::move(expected));
+  ASSERT_NE(nullptr, entry);
+  const auto &x = entry->second;
+  EXPECT_EQ(3, x.a);
+  EXPECT_EQ(4, x.b);
+}
+
+// Test Emplace function
+TEST(CuckooMapTest, Emplace) {
+  CuckooMap<uint32_t, CopyConstructorOnly> cuckoo;
+  auto *entry = cuckoo.Emplace(12, 5, 6);
+  ASSERT_NE(nullptr, entry);
+  const auto &x = entry->second;
+  EXPECT_EQ(5, x.a);
+  EXPECT_EQ(6, x.b);
+}
+
 // Test Find function
 TEST(CuckooMapTest, Find) {
   CuckooMap<uint32_t, uint16_t> cuckoo;
@@ -138,9 +241,7 @@ TEST(CuckooMapTest, Iterator) {
 TEST(CuckooMapTest, CollisionTest) {
   class BrokenHash {
    public:
-    bess::utils::HashResult operator()(const uint32_t) const {
-      return 9999999;
-    }
+    bess::utils::HashResult operator()(const uint32_t) const { return 9999999; }
   };
 
   CuckooMap<int, int, BrokenHash> cuckoo;
@@ -182,7 +283,6 @@ TEST(CuckooMapTest, RandomTest) {
   // check if the initial population succeeded
   for (size_t i = 0; i < array_size; i++) {
     auto ret = cuckoo.Find(i);
-    //std::cout << i << ' ' << idx << ' ' << truth[idx] << std::endl;
     if (truth[i] == 0) {
       EXPECT_EQ(nullptr, ret);
     } else {
@@ -209,7 +309,6 @@ TEST(CuckooMapTest, RandomTest) {
     } else {
       // 80% lookup
       auto ret = cuckoo.Find(idx);
-      //std::cout << i << ' ' << idx << ' ' << truth[idx] << std::endl;
       if (truth[idx] == 0) {
         EXPECT_EQ(nullptr, ret);
       } else {
@@ -220,4 +319,4 @@ TEST(CuckooMapTest, RandomTest) {
   }
 }
 
-}  // namespace (unnamed)
+}  // namespace