From 78071573aac62e34e76fe3e1ea1c747046e0a468 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Sat, 15 Apr 2023 20:22:50 -0400 Subject: [PATCH] Fix issue #117 - segfault when out of memory. when constructing an object to be inserted throws std::bad_alloc, the slot was mark as used (even though the object was not properly constructed), so eventually the destructor of a non-initialized object was called causing a segfault. Solution: mark the slot used only after the object is successfully constructed. --- parallel_hashmap/phmap.h | 131 +++++++++++++++++++++++---------------- 1 file changed, 77 insertions(+), 54 deletions(-) diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index 681a418..dc113a0 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -1526,13 +1526,28 @@ class raw_hash_set slot_type** slot_; }; + // Extension API: support for lazy emplace. + // Looks up key in the table. If found, returns the iterator to the element. + // Otherwise calls f with one argument of type raw_hash_set::constructor. f + // MUST call raw_hash_set::constructor with arguments as if a + // raw_hash_set::value_type is constructed, otherwise the behavior is + // undefined. + // + // For example: + // + // std::unordered_set s; + // // Makes ArenaStr even if "abc" is in the map. + // s.insert(ArenaString(&arena, "abc")); + // + // flat_hash_set s; + // // Makes ArenaStr only if "abc" is not in the map. + // s.lazy_emplace("abc", [&](const constructor& ctor) { + // ctor(&arena, "abc"); + // }); + // ----------------------------------------------------- template iterator lazy_emplace(const key_arg& key, F&& f) { - auto res = find_or_prepare_insert(key); - if (res.second) { - lazy_emplace_at(res.first, std::forward(f)); - } - return iterator_at(res.first); + return lazy_emplace_with_hash(key, this->hash(key), std::forward(f)); } template @@ -1540,6 +1555,7 @@ class raw_hash_set auto res = find_or_prepare_insert(key, hashval); if (res.second) { lazy_emplace_at(res.first, std::forward(f)); + this->set_ctrl(res.first, H2(hashval)); } return iterator_at(res.first); } @@ -1554,9 +1570,10 @@ class raw_hash_set template void emplace_single_with_hash(const key_arg& key, size_t hashval, F&& f) { auto res = find_or_prepare_insert(key, hashval); - if (res.second) + if (res.second) { lazy_emplace_at(res.first, std::forward(f)); - else + this->set_ctrl(res.first, H2(hashval)); + } else _erase(iterator_at(res.first)); } @@ -1888,6 +1905,7 @@ class raw_hash_set auto res = find_or_prepare_insert(key, hashval); if (res.second) { emplace_at(res.first, std::forward(args)...); + this->set_ctrl(res.first, H2(hashval)); } return {iterator_at(res.first), res.second}; } @@ -1915,9 +1933,11 @@ class raw_hash_set { template std::pair operator()(const K& key, Args&&...) && { - auto res = s.find_or_prepare_insert(key); + size_t hashval = s.hash(key); + auto res = s.find_or_prepare_insert(key, hashval); if (res.second) { PolicyTraits::transfer(&s.alloc_ref(), s.slots_ + res.first, &slot); + s.set_ctrl(res.first, H2(hashval)); } else if (do_destroy) { PolicyTraits::destroy(&s.alloc_ref(), &slot); } @@ -1936,6 +1956,7 @@ class raw_hash_set auto res = s.find_or_prepare_insert(key, hashval); if (res.second) { PolicyTraits::transfer(&s.alloc_ref(), s.slots_ + res.first, &slot); + s.set_ctrl(res.first, H2(hashval)); } else if (do_destroy) { PolicyTraits::destroy(&s.alloc_ref(), &slot); } @@ -2187,11 +2208,6 @@ class raw_hash_set return {prepare_insert(hashval), true}; } - template - std::pair find_or_prepare_insert(const K& key) { - return find_or_prepare_insert(key, this->hash(key)); - } - size_t prepare_insert(size_t hashval) PHMAP_ATTRIBUTE_NOINLINE { auto target = find_first_non_full(hashval); if (PHMAP_PREDICT_FALSE(growth_left() == 0 && @@ -2201,7 +2217,7 @@ class raw_hash_set } ++size_; growth_left() -= IsEmpty(ctrl_[target.offset]); - set_ctrl(target.offset, H2(hashval)); + // set_ctrl(target.offset, H2(hashval)); infoz_.RecordInsert(hashval, target.probe_length); return target.offset; } @@ -2230,6 +2246,23 @@ class raw_hash_set iterator iterator_at(size_t i) { return {ctrl_ + i, slots_ + i}; } const_iterator iterator_at(size_t i) const { return {ctrl_ + i, slots_ + i}; } +protected: + // Sets the control byte, and if `i < Group::kWidth`, set the cloned byte at + // the end too. + void set_ctrl(size_t i, ctrl_t h) { + assert(i < capacity_); + + if (IsFull(h)) { + SanitizerUnpoisonObject(slots_ + i); + } else { + SanitizerPoisonObject(slots_ + i); + } + + ctrl_[i] = h; + ctrl_[((i - Group::kWidth) & capacity_) + 1 + + ((Group::kWidth - 1) & capacity_)] = h; + } + private: friend struct RawHashSetTestOnlyAccess; @@ -2248,22 +2281,6 @@ class raw_hash_set growth_left() = CapacityToGrowth(capacity) - size_; } - // Sets the control byte, and if `i < Group::kWidth`, set the cloned byte at - // the end too. - void set_ctrl(size_t i, ctrl_t h) { - assert(i < capacity_); - - if (IsFull(h)) { - SanitizerUnpoisonObject(slots_ + i); - } else { - SanitizerPoisonObject(slots_ + i); - } - - ctrl_[i] = h; - ctrl_[((i - Group::kWidth) & capacity_) + 1 + - ((Group::kWidth - 1) & capacity_)] = h; - } - size_t& growth_left() { return settings_.template get<0>(); } template private: template std::pair insert_or_assign_impl(K&& k, V&& v) { - auto res = this->find_or_prepare_insert(k); - if (res.second) + size_t hashval = this->hash(k); + auto res = this->find_or_prepare_insert(k, hashval); + if (res.second) { this->emplace_at(res.first, std::forward(k), std::forward(v)); - else + this->set_ctrl(res.first, H2(hashval)); + } else Policy::value(&*this->iterator_at(res.first)) = std::forward(v); return {this->iterator_at(res.first), res.second}; } template std::pair try_emplace_impl(K&& k, Args&&... args) { - auto res = this->find_or_prepare_insert(k); - if (res.second) + size_t hashval = this->hash(k); + auto res = this->find_or_prepare_insert(k, hashval); + if (res.second) { this->emplace_at(res.first, std::piecewise_construct, std::forward_as_tuple(std::forward(k)), std::forward_as_tuple(std::forward(args)...)); + this->set_ctrl(res.first, H2(hashval)); + } return {this->iterator_at(res.first), res.second}; } }; @@ -3295,12 +3317,14 @@ class parallel_hash_set // --------------------------------------------------------------------------------------- template bool lazy_emplace_l(const key_arg& key, FExists&& fExists, FEmplace&& fEmplace) { + size_t hashval = this->hash(key); typename Lockable::UniqueLock m; - auto res = this->find_or_prepare_insert(key, m); + auto res = this->find_or_prepare_insert_with_hash(hashval, key, m); Inner* inner = std::get<0>(res); - if (std::get<2>(res)) + if (std::get<2>(res)) { inner->set_.lazy_emplace_at(std::get<1>(res), std::forward(fEmplace)); - else { + inner->set_.set_ctrl(std::get<1>(res), H2(hashval)); + } else { auto it = this->iterator_at(inner, inner->set_.iterator_at(std::get<1>(res))); std::forward(fExists)(const_cast(*it)); // in case of the set, non "key" part of value_type can be changed } @@ -3979,14 +4003,16 @@ class parallel_hash_map : public parallel_hash_set bool try_emplace_l(K&& k, F&& f, Args&&... args) { + size_t hashval = this->hash(k); typename Lockable::UniqueLock m; - auto res = this->find_or_prepare_insert(k, m); + auto res = this->find_or_prepare_insert_with_hash(hashval, k, m); typename Base::Inner *inner = std::get<0>(res); - if (std::get<2>(res)) + if (std::get<2>(res)) { inner->set_.emplace_at(std::get<1>(res), std::piecewise_construct, std::forward_as_tuple(std::forward(k)), std::forward_as_tuple(std::forward(args)...)); - else { + inner->set_.set_ctrl(std::get<1>(res), H2(hashval)); + } else { auto it = this->iterator_at(inner, inner->set_.iterator_at(std::get<1>(res))); std::forward(f)(const_cast(*it)); // in case of the set, non "key" part of value_type can be changed } @@ -4009,12 +4035,14 @@ class parallel_hash_map : public parallel_hash_set std::pair insert_or_assign_impl(K&& k, V&& v) { + size_t hashval = this->hash(k); typename Lockable::UniqueLock m; - auto res = this->find_or_prepare_insert(k, m); + auto res = this->find_or_prepare_insert_with_hash(hashval, k, m); typename Base::Inner *inner = std::get<0>(res); - if (std::get<2>(res)) + if (std::get<2>(res)) { inner->set_.emplace_at(std::get<1>(res), std::forward(k), std::forward(v)); - else + inner->set_.set_ctrl(std::get<1>(res), H2(hashval)); + } else Policy::value(&*inner->set_.iterator_at(std::get<1>(res))) = std::forward(v); return {this->iterator_at(inner, inner->set_.iterator_at(std::get<1>(res))), std::get<2>(res)}; @@ -4022,15 +4050,8 @@ class parallel_hash_map : public parallel_hash_set std::pair try_emplace_impl(K&& k, Args&&... args) { - typename Lockable::UniqueLock m; - auto res = this->find_or_prepare_insert(k, m); - typename Base::Inner *inner = std::get<0>(res); - if (std::get<2>(res)) - inner->set_.emplace_at(std::get<1>(res), std::piecewise_construct, - std::forward_as_tuple(std::forward(k)), - std::forward_as_tuple(std::forward(args)...)); - return {this->iterator_at(inner, inner->set_.iterator_at(std::get<1>(res))), - std::get<2>(res)}; + return try_emplace_impl_with_hash(this->hash(k), std::forward(k), + std::forward(args)...); } template @@ -4038,10 +4059,12 @@ class parallel_hash_map : public parallel_hash_setfind_or_prepare_insert_with_hash(hashval, k, m); typename Base::Inner *inner = std::get<0>(res); - if (std::get<2>(res)) + if (std::get<2>(res)) { inner->set_.emplace_at(std::get<1>(res), std::piecewise_construct, std::forward_as_tuple(std::forward(k)), std::forward_as_tuple(std::forward(args)...)); + inner->set_.set_ctrl(std::get<1>(res), H2(hashval)); + } return {this->iterator_at(inner, inner->set_.iterator_at(std::get<1>(res))), std::get<2>(res)}; }