diff --git a/core/utils/cuckoo_map.h b/core/utils/cuckoo_map.h index 0643d9cde..a0fb4b496 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(), std::forward(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..634199773 100644 --- a/core/utils/cuckoo_map_test.cc +++ b/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 { + std::size_t operator()(const CopyConstructorOnly &t) const noexcept { + return std::hash()(t.a + t.b); // doesn't need to be a good one... + } +}; + +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... + } +}; + namespace { using bess::utils::CuckooMap; @@ -45,6 +91,63 @@ TEST(CuckooMapTest, Insert) { 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; + 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 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(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 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 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