From 6a577c46870af2a714eea8efecb57df4e8d7c612 Mon Sep 17 00:00:00 2001 From: Vincent Hengel Date: Sun, 27 Sep 2026 08:37:03 +0200 Subject: [PATCH] fix(base): UnorderedMap inserts no longer duplicate a key past a tombstone --- base/containers/unordered_map.h | 131 ++++++++++++-------------- base/containers/unordered_map_test.cc | 43 +++++++++ 2 files changed, 102 insertions(+), 72 deletions(-) diff --git a/base/containers/unordered_map.h b/base/containers/unordered_map.h index 44ffc2ea..238998b4 100644 --- a/base/containers/unordered_map.h +++ b/base/containers/unordered_map.h @@ -173,6 +173,37 @@ class UnorderedMap { return ~mem_size(0); } + // Returns the slot holding |key| (second = false), or the slot a new |key| + // goes into (second = true). The probe must reach an empty slot before + // reusing a tombstone, since |key| may sit further along the chain. + Pair FindOrPrepareInsert(const K& key) { + if (bucket_count_ == 0 || + (size_ + tombstone_count_ + 1) > static_cast(bucket_count_ * kMaxLoadFactor)) { + GrowAndRehash(); + } + + constexpr mem_size kNone = ~mem_size(0); + mem_size tombstone = kNone; + mem_size idx = BucketFor(key); + while (slots_[idx].state != kEmpty) { + if (slots_[idx].state == kDeleted) { + if (tombstone == kNone) + tombstone = idx; + } else if (equal_(*SlotKey(slots_[idx]), key)) { + return {idx, false}; + } + idx = (idx + 1) & (bucket_count_ - 1); + } + return {tombstone != kNone ? tombstone : idx, true}; + } + + void MarkOccupied(Slot& s) { + if (s.state == kDeleted) + --tombstone_count_; + s.state = kOccupied; + ++size_; + } + public: UnorderedMap() = default; @@ -264,25 +295,14 @@ class UnorderedMap { // Element access V& operator[](const K& key) { - if (bucket_count_ == 0 || - (size_ + tombstone_count_ + 1) > static_cast(bucket_count_ * kMaxLoadFactor)) { - GrowAndRehash(); - } - - mem_size idx = BucketFor(key); - while (true) { - if (slots_[idx].state == kEmpty || slots_[idx].state == kDeleted) { - ::new (&slots_[idx].key_storage[0]) K(key); - ::new (&slots_[idx].val_storage[0]) V(); - slots_[idx].state = kOccupied; - ++size_; - return *SlotVal(slots_[idx]); - } - if (slots_[idx].state == kOccupied && equal_(*SlotKey(slots_[idx]), key)) { - return *SlotVal(slots_[idx]); - } - idx = (idx + 1) & (bucket_count_ - 1); + const Pair slot = FindOrPrepareInsert(key); + Slot& s = slots_[slot.first]; + if (slot.second) { + ::new (&s.key_storage[0]) K(key); + ::new (&s.val_storage[0]) V(); + MarkOccupied(s); } + return *SlotVal(s); } V* find(const K& key) { @@ -335,47 +355,25 @@ class UnorderedMap { // Insert or assign Pair insert(const K& key, const V& value) { - if (bucket_count_ == 0 || - (size_ + tombstone_count_ + 1) > static_cast(bucket_count_ * kMaxLoadFactor)) { - GrowAndRehash(); - } - - mem_size idx = BucketFor(key); - while (true) { - if (slots_[idx].state == kEmpty || slots_[idx].state == kDeleted) { - ::new (&slots_[idx].key_storage[0]) K(key); - ::new (&slots_[idx].val_storage[0]) V(value); - slots_[idx].state = kOccupied; - ++size_; - return {SlotVal(slots_[idx]), true}; - } - if (slots_[idx].state == kOccupied && equal_(*SlotKey(slots_[idx]), key)) { - return {SlotVal(slots_[idx]), false}; // already exists - } - idx = (idx + 1) & (bucket_count_ - 1); + const Pair slot = FindOrPrepareInsert(key); + Slot& s = slots_[slot.first]; + if (slot.second) { + ::new (&s.key_storage[0]) K(key); + ::new (&s.val_storage[0]) V(value); + MarkOccupied(s); } + return {SlotVal(s), slot.second}; } Pair insert(const K& key, V&& value) { - if (bucket_count_ == 0 || - (size_ + tombstone_count_ + 1) > static_cast(bucket_count_ * kMaxLoadFactor)) { - GrowAndRehash(); - } - - mem_size idx = BucketFor(key); - while (true) { - if (slots_[idx].state == kEmpty || slots_[idx].state == kDeleted) { - ::new (&slots_[idx].key_storage[0]) K(key); - ::new (&slots_[idx].val_storage[0]) V(base::move(value)); - slots_[idx].state = kOccupied; - ++size_; - return {SlotVal(slots_[idx]), true}; - } - if (slots_[idx].state == kOccupied && equal_(*SlotKey(slots_[idx]), key)) { - return {SlotVal(slots_[idx]), false}; - } - idx = (idx + 1) & (bucket_count_ - 1); + const Pair slot = FindOrPrepareInsert(key); + Slot& s = slots_[slot.first]; + if (slot.second) { + ::new (&s.key_storage[0]) K(key); + ::new (&s.val_storage[0]) V(base::move(value)); + MarkOccupied(s); } + return {SlotVal(s), slot.second}; } // emplace under the std spelling; identical semantics (an existing key is @@ -387,25 +385,14 @@ class UnorderedMap { template Pair emplace(const K& key, TArgs&&... args) { - if (bucket_count_ == 0 || - (size_ + tombstone_count_ + 1) > static_cast(bucket_count_ * kMaxLoadFactor)) { - GrowAndRehash(); - } - - mem_size idx = BucketFor(key); - while (true) { - if (slots_[idx].state == kEmpty || slots_[idx].state == kDeleted) { - ::new (&slots_[idx].key_storage[0]) K(key); - ::new (&slots_[idx].val_storage[0]) V(base::forward(args)...); - slots_[idx].state = kOccupied; - ++size_; - return {SlotVal(slots_[idx]), true}; - } - if (slots_[idx].state == kOccupied && equal_(*SlotKey(slots_[idx]), key)) { - return {SlotVal(slots_[idx]), false}; - } - idx = (idx + 1) & (bucket_count_ - 1); + const Pair slot = FindOrPrepareInsert(key); + Slot& s = slots_[slot.first]; + if (slot.second) { + ::new (&s.key_storage[0]) K(key); + ::new (&s.val_storage[0]) V(base::forward(args)...); + MarkOccupied(s); } + return {SlotVal(s), slot.second}; } void clear() { diff --git a/base/containers/unordered_map_test.cc b/base/containers/unordered_map_test.cc index 3af3caf0..b4acb681 100644 --- a/base/containers/unordered_map_test.cc +++ b/base/containers/unordered_map_test.cc @@ -98,6 +98,49 @@ TEST(UnorderedMapTest, LookupSurvivesTombstones) { EXPECT_EQ(map.size(), 32u); } +// Every key lands in bucket 0, so key 2 sits one slot past key 1. +struct CollidingHash { + mem_size operator()(i32) const { return 0; } +}; + +TEST(UnorderedMapTest, SubscriptFindsAKeyPastATombstone) { + base::UnorderedMap map; + map[1] = 10; + map[2] = 20; + map.erase(1); + + map[2] = 21; + + EXPECT_EQ(map.size(), 1u); + EXPECT_EQ(*map.find(2), 21); + map.erase(2); + EXPECT_EQ(map.find(2), nullptr); +} + +TEST(UnorderedMapTest, InsertAndEmplaceFindAKeyPastATombstone) { + base::UnorderedMap map; + map.insert(1, 10); + map.insert(2, 20); + map.erase(1); + + EXPECT_FALSE(map.insert(2, 99).second); + EXPECT_FALSE(map.emplace(2, 99).second); + + EXPECT_EQ(map.size(), 1u); + EXPECT_EQ(*map.find(2), 20); +} + +TEST(UnorderedMapTest, InsertReusesATombstone) { + base::UnorderedMap map; + map.insert(1, 10); + const mem_size capacity = map.remaining_capacity(); + map.erase(1); + + map.insert(1, 11); + + EXPECT_EQ(map.remaining_capacity(), capacity); +} + TEST(UnorderedMapTest, GrowsAndRehashes) { base::UnorderedMap map; for (i32 i = 0; i < 1000; ++i) map.insert(i, i);