From c7273bb84ec487b46674a8ede8924158e7725994 Mon Sep 17 00:00:00 2001 From: Melvin Walls Date: Wed, 15 Aug 2018 15:50:45 -0700 Subject: [PATCH 1/3] utils/cuckoo_map: add Emplace, move-based Insert --- core/utils/cuckoo_map.h | 63 ++++++++++++++++++++++---------- core/utils/cuckoo_map_test.cc | 69 ++++++++++++++++++++++++++++++++--- 2 files changed, 106 insertions(+), 26 deletions(-) diff --git a/core/utils/cuckoo_map.h b/core/utils/cuckoo_map.h index 0643d9cde..d08b79b53 100644 --- a/core/utils/cuckoo_map.h +++ b/core/utils/cuckoo_map.h @@ -76,6 +76,7 @@ template , class CuckooMap { public: typedef std::pair Entry; + class iterator { public: using difference_type = std::ptrdiff_t; @@ -173,17 +174,15 @@ 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 + 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)...); return entry; } @@ -191,8 +190,8 @@ class CuckooMap { int trials = 0; - while ((entry = AddEntry(primary, secondary, key, value, hasher)) == - nullptr) { + while ((entry = EmplaceEntry(primary, secondary, key, hasher, + std::forward(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::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 + Entry* Insert(const K& key, VV&& value, const H& hasher = H(), + const E& eq = E()) { + return DoEmplace(key, hasher, eq, std::forward(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 + Entry* Emplace(const K& key, Args&&... args) { + return DoEmplace(key, H(), E(), 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 + 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)...); 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 + 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)...)) != 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)...)) != nullptr) { return entry; } @@ -501,12 +522,14 @@ class CuckooMap { } // Resize the space of buckets, and rehash existing entries + template void ExpandBuckets(const H& hasher, const E& eq) { CuckooMap 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(e.second)); if (!ret) { return; } diff --git a/core/utils/cuckoo_map_test.cc b/core/utils/cuckoo_map_test.cc index 096ea53f2..a5ebc65b0 100644 --- a/core/utils/cuckoo_map_test.cc +++ b/core/utils/cuckoo_map_test.cc @@ -45,6 +45,67 @@ TEST(CuckooMapTest, Insert) { EXPECT_EQ(cuckoo.Insert(1, 1)->second, 1); } +// Only default and move constructible +struct Foo { + Foo() : a(), b(), c() {} + Foo(Foo &&other) : a(other.a), b(other.b), c(other.c) {} + Foo(const Foo &) = delete; + + explicit Foo(int aa, int bb, int cc) : a(aa), b(bb), c(cc) {} + + int a; + int b; + int c; +}; + +// Only default and copy constructible +struct Bar { + Bar() : a(), b(), c() {} + Bar(const Bar &other) : a(other.a), b(other.b), c(other.c) {} + Bar(Bar &&other) = delete; + + explicit Bar(int aa, int bb, int cc) : a(aa), b(bb), c(cc) {} + + int a; + int b; + int c; +}; + +// Test insertion with move +TEST(CuckooMapTest, MoveInsert) { + CuckooMap cuckoo; + Foo expected = Foo(1, 2, 3); + auto *entry = cuckoo.Insert(1, std::move(expected)); + ASSERT_NE(nullptr, entry); + const Foo &x = entry->second; + EXPECT_EQ(1, x.a); + EXPECT_EQ(2, x.b); + EXPECT_EQ(3, x.c); +} + +// Test insertion with copy +TEST(CuckooMapTest, CopyInsert) { + CuckooMap cuckoo; + Bar expected = Bar(1, 2, 3); + auto *entry = cuckoo.Insert(1, expected); + ASSERT_NE(nullptr, entry); + const Bar &x = entry->second; + EXPECT_EQ(1, x.a); + EXPECT_EQ(2, x.b); + EXPECT_EQ(3, x.c); +} + +// Test Emplace function +TEST(CuckooMapTest, Emplace) { + CuckooMap cuckoo; + auto *entry = cuckoo.Emplace(1, 1, 2, 3); + ASSERT_NE(nullptr, entry); + const Foo &x = entry->second; + EXPECT_EQ(1, x.a); + EXPECT_EQ(2, x.b); + EXPECT_EQ(3, x.c); +} + // Test Find function TEST(CuckooMapTest, Find) { CuckooMap cuckoo; @@ -138,9 +199,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 cuckoo; @@ -182,7 +241,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 +267,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 +277,4 @@ TEST(CuckooMapTest, RandomTest) { } } -} // namespace (unnamed) +} // namespace From 7c32f4b4762db3f950d1c898d4f0e6122fefc69c Mon Sep 17 00:00:00 2001 From: Sangjin Han Date: Sun, 19 Aug 2018 22:08:20 -0700 Subject: [PATCH 2/3] utils: do perfect forwarding for args of Emplace() --- core/utils/cuckoo_map.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/utils/cuckoo_map.h b/core/utils/cuckoo_map.h index d08b79b53..a0fb4b496 100644 --- a/core/utils/cuckoo_map.h +++ b/core/utils/cuckoo_map.h @@ -222,7 +222,7 @@ class CuckooMap { // called. template Entry* Emplace(const K& key, Args&&... args) { - return DoEmplace(key, H(), E(), args...); + return DoEmplace(key, H(), E(), std::forward(args)...); } // Find the pointer to the stored value by the key. From f9b080c6c0c928cbd62cbc0f76267f8d01134a72 Mon Sep 17 00:00:00 2001 From: Sangjin Han Date: Sun, 19 Aug 2018 22:14:06 -0700 Subject: [PATCH 3/3] utils: revise unittest for CuckooMap --- core/utils/cuckoo_map_test.cc | 134 ++++++++++++++++++++++------------ 1 file changed, 88 insertions(+), 46 deletions(-) diff --git a/core/utils/cuckoo_map_test.cc b/core/utils/cuckoo_map_test.cc index a5ebc65b0..634199773 100644 --- a/core/utils/cuckoo_map_test.cc +++ b/core/utils/cuckoo_map_test.cc @@ -33,77 +33,119 @@ #include "random.h" -namespace { +struct CopyConstructorOnly { + // FIXME: CuckooMap should work without this default constructor + CopyConstructorOnly() = default; + CopyConstructorOnly(CopyConstructorOnly &&other) = delete; -using bess::utils::CuckooMap; + CopyConstructorOnly(int aa, int bb): a(aa), b(bb) {} + CopyConstructorOnly(const CopyConstructorOnly &other) + : a(other.a), b(other.b) {} -// Test Insert function -TEST(CuckooMapTest, Insert) { - CuckooMap cuckoo; - EXPECT_EQ(cuckoo.Insert(1, 99)->second, 99); - EXPECT_EQ(cuckoo.Insert(2, 98)->second, 98); - EXPECT_EQ(cuckoo.Insert(1, 1)->second, 1); -} + int a; + int b; +}; -// Only default and move constructible -struct Foo { - Foo() : a(), b(), c() {} - Foo(Foo &&other) : a(other.a), b(other.b), c(other.c) {} - Foo(const Foo &) = delete; +struct MoveConstructorOnly { + // FIXME: CuckooMap should work without this default constructor + MoveConstructorOnly() = default; + MoveConstructorOnly(const MoveConstructorOnly &other) = delete; - explicit Foo(int aa, int bb, int cc) : a(aa), b(bb), c(cc) {} + 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; - int c; }; -// Only default and copy constructible -struct Bar { - Bar() : a(), b(), c() {} - Bar(const Bar &other) : a(other.a), b(other.b), c(other.c) {} - Bar(Bar &&other) = delete; +// C++ has no clean way to specialize templates for derived typess... +// so we just define a hash functor for each. - explicit Bar(int aa, int bb, int cc) : a(aa), b(bb), c(cc) {} +template <> +struct std::hash { + std::size_t operator()(const CopyConstructorOnly &t) const noexcept { + return std::hash()(t.a + t.b); // doesn't need to be a good one... + } +}; - int a; - int b; - int c; +template <> +struct std::hash { + std::size_t operator()(const MoveConstructorOnly &t) const noexcept { + return std::hash()(t.a * t.b); // doesn't need to be a good one... + } }; -// Test insertion with move -TEST(CuckooMapTest, MoveInsert) { - CuckooMap cuckoo; - Foo expected = Foo(1, 2, 3); - auto *entry = cuckoo.Insert(1, std::move(expected)); - ASSERT_NE(nullptr, entry); - const Foo &x = entry->second; - EXPECT_EQ(1, x.a); - EXPECT_EQ(2, x.b); - EXPECT_EQ(3, x.c); +namespace { + +using bess::utils::CuckooMap; + +// Test Insert function +TEST(CuckooMapTest, Insert) { + CuckooMap cuckoo; + EXPECT_EQ(cuckoo.Insert(1, 99)->second, 99); + EXPECT_EQ(cuckoo.Insert(2, 98)->second, 98); + EXPECT_EQ(cuckoo.Insert(1, 1)->second, 1); +} + +template +void CompileTimeInstantiation() { + std::map m1; + std::map m2; + std::map m3; + std::unordered_map u1; + std::unordered_map u2; + std::unordered_map u3; + std::vector v1; + + // FIXME: currently, CuckooMap does not support types without a default + // constructor. The following will fail with the current code. + // CuckooMap c1; + // CuckooMap c2; + // CuckooMap 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(); + CompileTimeInstantiation(); } // Test insertion with copy TEST(CuckooMapTest, CopyInsert) { - CuckooMap cuckoo; - Bar expected = Bar(1, 2, 3); - auto *entry = cuckoo.Insert(1, expected); + CuckooMap cuckoo; + auto expected = CopyConstructorOnly(1, 2); + auto *entry = cuckoo.Insert(10, expected); ASSERT_NE(nullptr, entry); - const Bar &x = entry->second; + const auto &x = entry->second; EXPECT_EQ(1, x.a); EXPECT_EQ(2, x.b); - EXPECT_EQ(3, x.c); +} + +// Test insertion with move +TEST(CuckooMapTest, MoveInsert) { + CuckooMap 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 cuckoo; - auto *entry = cuckoo.Emplace(1, 1, 2, 3); + CuckooMap cuckoo; + auto *entry = cuckoo.Emplace(12, 5, 6); ASSERT_NE(nullptr, entry); - const Foo &x = entry->second; - EXPECT_EQ(1, x.a); - EXPECT_EQ(2, x.b); - EXPECT_EQ(3, x.c); + const auto &x = entry->second; + EXPECT_EQ(5, x.a); + EXPECT_EQ(6, x.b); } // Test Find function