[libcxx-commits] [libcxx] 253e435 - Reapply "[libc++] Optimize __hash_table::erase(iterator, iterator)" (#162850)
via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Oct 21 01:20:11 PDT 2025
Author: Nikolas Klauser
Date: 2025-10-21T10:20:06+02:00
New Revision: 253e43590842bffcc6950cc517a7f89cafe5ec69
URL: https://github.com/llvm/llvm-project/commit/253e43590842bffcc6950cc517a7f89cafe5ec69
DIFF: https://github.com/llvm/llvm-project/commit/253e43590842bffcc6950cc517a7f89cafe5ec69.diff
LOG: Reapply "[libc++] Optimize __hash_table::erase(iterator, iterator)" (#162850)
This reapplication fixes the use after free caused by not properly
updating the bucket list in one case.
Original commit message:
Instead of just calling the single element `erase` on every element of
the range, we can combine some of the operations in a custom
implementation. Specifically, we don't need to search for the previous
node or re-link the list every iteration. Removing this unnecessary work
results in some nice performance improvements:
```
-----------------------------------------------------------------------------------------------------------------------
Benchmark old new
-----------------------------------------------------------------------------------------------------------------------
std::unordered_set<int>::erase(iterator, iterator) (erase half the container)/0 457 ns 459 ns
std::unordered_set<int>::erase(iterator, iterator) (erase half the container)/32 995 ns 626 ns
std::unordered_set<int>::erase(iterator, iterator) (erase half the container)/1024 18196 ns 7995 ns
std::unordered_set<int>::erase(iterator, iterator) (erase half the container)/8192 124722 ns 70125 ns
std::unordered_set<std::string>::erase(iterator, iterator) (erase half the container)/0 456 ns 461 ns
std::unordered_set<std::string>::erase(iterator, iterator) (erase half the container)/32 1183 ns 769 ns
std::unordered_set<std::string>::erase(iterator, iterator) (erase half the container)/1024 27827 ns 18614 ns
std::unordered_set<std::string>::erase(iterator, iterator) (erase half the container)/8192 266681 ns 226107 ns
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/0 455 ns 462 ns
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/32 996 ns 659 ns
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/1024 15963 ns 8108 ns
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/8192 136493 ns 71848 ns
std::unordered_multiset<int>::erase(iterator, iterator) (erase half the container)/0 454 ns 455 ns
std::unordered_multiset<int>::erase(iterator, iterator) (erase half the container)/32 985 ns 703 ns
std::unordered_multiset<int>::erase(iterator, iterator) (erase half the container)/1024 16277 ns 9085 ns
std::unordered_multiset<int>::erase(iterator, iterator) (erase half the container)/8192 125736 ns 82710 ns
std::unordered_multimap<int, int>::erase(iterator, iterator) (erase half the container)/0 457 ns 454 ns
std::unordered_multimap<int, int>::erase(iterator, iterator) (erase half the container)/32 1091 ns 646 ns
std::unordered_multimap<int, int>::erase(iterator, iterator) (erase half the container)/1024 17784 ns 7664 ns
std::unordered_multimap<int, int>::erase(iterator, iterator) (erase half the container)/8192 127098 ns 72806 ns
```
This reverts commit acc3a6234a91369b818fdd6482ded0ac32d8ffa6.
Added:
Modified:
libcxx/docs/ReleaseNotes/22.rst
libcxx/include/__hash_table
libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp
libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/docs/ReleaseNotes/22.rst b/libcxx/docs/ReleaseNotes/22.rst
index ada8b413e1259..e95cbc0c0f5b5 100644
--- a/libcxx/docs/ReleaseNotes/22.rst
+++ b/libcxx/docs/ReleaseNotes/22.rst
@@ -62,6 +62,7 @@ Improvements and New Features
has been improved by up to 3x
- The performance of ``insert(iterator, iterator)`` of ``map``, ``set``, ``multimap`` and ``multiset`` has been improved
by up to 2.5x
+- The performance of ``erase(iterator, iterator)`` in the unordered containers has been improved by up to 1.9x
- The performance of ``map::insert_or_assign`` has been improved by up to 2x
- ``ofstream::write`` has been optimized to pass through large strings to system calls directly instead of copying them
in chunks into a buffer.
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index 6b65e738fef3b..5432abb4ab39d 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -1037,7 +1037,21 @@ private:
}
_LIBCPP_HIDE_FROM_ABI void __move_assign_alloc(__hash_table&, false_type) _NOEXCEPT {}
- _LIBCPP_HIDE_FROM_ABI void __deallocate_node(__next_pointer __np) _NOEXCEPT;
+ _LIBCPP_HIDE_FROM_ABI void __deallocate_node(__node_pointer __nd) _NOEXCEPT {
+ auto& __alloc = __node_alloc();
+ __node_traits::destroy(__alloc, std::addressof(__nd->__get_value()));
+ std::__destroy_at(std::__to_address(__nd));
+ __node_traits::deallocate(__alloc, __nd, 1);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI void __deallocate_node_list(__next_pointer __np) _NOEXCEPT {
+ while (__np != nullptr) {
+ __next_pointer __next = __np->__next_;
+ __deallocate_node(__np->__upcast());
+ __np = __next;
+ }
+ }
+
_LIBCPP_HIDE_FROM_ABI __next_pointer __detach() _NOEXCEPT;
template <class _From, class _ValueT = _Tp, __enable_if_t<__is_hash_value_type<_ValueT>::value, int> = 0>
@@ -1175,7 +1189,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::~__hash_table() {
static_assert(is_copy_constructible<hasher>::value, "Hasher must be copy-constructible.");
#endif
- __deallocate_node(__first_node_.__next_);
+ __deallocate_node_list(__first_node_.__next_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
@@ -1251,7 +1265,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::operator=(const __hash_table& __other)
// At this point we either have consumed the whole incoming hash table, or we don't have any more nodes to reuse in
// the destination. Either continue with constructing new nodes, or deallocate the left over nodes.
if (__own_iter->__next_) {
- __deallocate_node(__own_iter->__next_);
+ __deallocate_node_list(__own_iter->__next_);
__own_iter->__next_ = nullptr;
} else {
__copy_construct(__other_iter, __own_iter, __current_chash);
@@ -1262,19 +1276,6 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::operator=(const __hash_table& __other)
return *this;
}
-template <class _Tp, class _Hash, class _Equal, class _Alloc>
-void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__deallocate_node(__next_pointer __np) _NOEXCEPT {
- __node_allocator& __na = __node_alloc();
- while (__np != nullptr) {
- __next_pointer __next = __np->__next_;
- __node_pointer __real_np = __np->__upcast();
- __node_traits::destroy(__na, std::addressof(__real_np->__get_value()));
- std::__destroy_at(std::addressof(*__real_np));
- __node_traits::deallocate(__na, __real_np, 1);
- __np = __next;
- }
-}
-
template <class _Tp, class _Hash, class _Equal, class _Alloc>
typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::__next_pointer
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__detach() _NOEXCEPT {
@@ -1318,7 +1319,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__move_assign(__hash_table& __u,
max_load_factor() = __u.max_load_factor();
if (bucket_count() != 0) {
__next_pointer __cache = __detach();
- auto __guard = std::__make_scope_guard([&] { __deallocate_node(__cache); });
+ auto __guard = std::__make_scope_guard([&] { __deallocate_node_list(__cache); });
const_iterator __i = __u.begin();
while (__cache != nullptr && __u.size() != 0) {
__assign_value(__cache->__upcast()->__get_value(), std::move(__u.remove(__i++)->__get_value()));
@@ -1353,7 +1354,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_unique(_InputIterator __
if (bucket_count() != 0) {
__next_pointer __cache = __detach();
- auto __guard = std::__make_scope_guard([&] { __deallocate_node(__cache); });
+ auto __guard = std::__make_scope_guard([&] { __deallocate_node_list(__cache); });
for (; __cache != nullptr && __first != __last; ++__first) {
__assign_value(__cache->__upcast()->__get_value(), *__first);
__next_pointer __next = __cache->__next_;
@@ -1374,7 +1375,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_multi(_InputIterator __f
"__assign_multi may only be called with the containers value type or the nodes value type");
if (bucket_count() != 0) {
__next_pointer __cache = __detach();
- auto __guard = std::__make_scope_guard([&] { __deallocate_node(__cache); });
+ auto __guard = std::__make_scope_guard([&] { __deallocate_node_list(__cache); });
for (; __cache != nullptr && __first != __last; ++__first) {
__assign_value(__cache->__upcast()->__get_value(), *__first);
__next_pointer __next = __cache->__next_;
@@ -1413,7 +1414,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::end() const _NOEXCEPT {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
void __hash_table<_Tp, _Hash, _Equal, _Alloc>::clear() _NOEXCEPT {
if (size() > 0) {
- __deallocate_node(__first_node_.__next_);
+ __deallocate_node_list(__first_node_.__next_);
__first_node_.__next_ = nullptr;
size_type __bc = bucket_count();
for (size_type __i = 0; __i < __bc; ++__i)
@@ -1873,12 +1874,61 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __p) {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
__hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __first, const_iterator __last) {
- for (const_iterator __p = __first; __first != __last; __p = __first) {
- ++__first;
- erase(__p);
+ if (__first == __last)
+ return iterator(__last.__node_);
+
+ // current node
+ __next_pointer __current = __first.__node_;
+ size_type __bucket_count = bucket_count();
+ size_t __chash = std::__constrain_hash(__current->__hash(), __bucket_count);
+ // find previous node
+ __next_pointer __before_first = __bucket_list_[__chash];
+ for (; __before_first->__next_ != __current; __before_first = __before_first->__next_)
+ ;
+
+ __next_pointer __last_node = __last.__node_;
+
+ // If __before_first is in the same bucket (i.e. the first element we erase is not the first in the bucket), clear
+ // this bucket first without re-linking it
+ if (__before_first != __first_node_.__ptr() &&
+ std::__constrain_hash(__before_first->__hash(), __bucket_count) == __chash) {
+ while (__current != __last_node) {
+ auto __next = __current->__next_;
+ __deallocate_node(__current->__upcast());
+ __current = __next;
+ --__size_;
+
+ if (__next) {
+ if (auto __next_chash = std::__constrain_hash(__next->__hash(), __bucket_count); __next_chash != __chash) {
+ __bucket_list_[__next_chash] = __before_first;
+ __chash = __next_chash;
+ break;
+ }
+ }
+ }
+ }
+
+ while (__current != __last_node) {
+ auto __next = __current->__next_;
+ __deallocate_node(__current->__upcast());
+ __current = __next;
+ --__size_;
+
+ // When switching buckets, set the old bucket to be empty and update the next bucket to have __before_first as its
+ // before-first element
+ if (__next) {
+ if (auto __next_chash = std::__constrain_hash(__next->__hash(), __bucket_count); __next_chash != __chash) {
+ __bucket_list_[__chash] = nullptr;
+ __bucket_list_[__next_chash] = __before_first;
+ __chash = __next_chash;
+ }
+ }
}
- __next_pointer __np = __last.__node_;
- return iterator(__np);
+
+ // re-link __before_first with __last
+ __before_first->__next_ = __current;
+
+ return iterator(__last.__node_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
diff --git a/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp b/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp
index f8a2bdd3fee73..38b75c0c1986b 100644
--- a/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.multimap/unord.multimap.modifiers/erase_range.pass.cpp
@@ -91,6 +91,37 @@ int main(int, char**) {
assert(c.size() == 0);
assert(k == c.end());
}
+ { // Make sure we're properly destroying the elements when erasing
+ { // When erasing part of a bucket
+ std::unordered_multimap<int, std::string> map;
+ map.insert(std::make_pair(1, "This is a long string to make sure ASan can detect a memory leak"));
+ map.insert(std::make_pair(1, "This is another long string to make sure ASan can detect a memory leak"));
+ map.erase(++map.begin(), map.end());
+ }
+ { // When erasing the whole bucket
+ std::unordered_multimap<int, std::string> map;
+ map.insert(std::make_pair(1, "This is a long string to make sure ASan can detect a memory leak"));
+ map.insert(std::make_pair(1, "This is another long string to make sure ASan can detect a memory leak"));
+ map.erase(map.begin(), map.end());
+ }
+ }
+ { // Make sure that we're properly updating the bucket list when starting within a bucket
+ struct MyHash {
+ size_t operator()(size_t v) const { return v; }
+ };
+ std::unordered_multimap<size_t, size_t, MyHash> map;
+ size_t collision_val = 2 + map.bucket_count(); // try to get a bucket collision
+ map.rehash(3);
+ map.insert(std::pair<size_t, size_t>(1, 1));
+ map.insert(std::pair<size_t, size_t>(collision_val, 1));
+ map.insert(std::pair<size_t, size_t>(2, 1));
+ LIBCPP_ASSERT(map.bucket(2) == map.bucket(collision_val));
+
+ auto erase = map.equal_range(2);
+ map.erase(erase.first, erase.second);
+ for (const auto& v : map)
+ assert(v.first == 1 || v.first == collision_val);
+ }
#if TEST_STD_VER >= 11
{
typedef std::unordered_multimap<int,
diff --git a/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp b/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp
index 62724fdbeead8..3bc686ec2d86e 100644
--- a/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.multiset/erase_range.pass.cpp
@@ -47,6 +47,23 @@ int main(int, char**) {
assert(c.size() == 0);
assert(k == c.end());
}
+ { // Make sure that we're properly updating the bucket list when starting within a bucket
+ struct MyHash {
+ size_t operator()(size_t v) const { return v; }
+ };
+ std::unordered_multiset<size_t, MyHash> map;
+ size_t collision_val = 2 + map.bucket_count(); // try to get a bucket collision
+ map.rehash(3);
+ map.insert(1);
+ map.insert(collision_val);
+ map.insert(2);
+ LIBCPP_ASSERT(map.bucket(2) == map.bucket(collision_val));
+
+ auto erase = map.equal_range(2);
+ map.erase(erase.first, erase.second);
+ for (const auto& v : map)
+ assert(v == 1 || v == collision_val);
+ }
#if TEST_STD_VER >= 11
{
typedef std::unordered_multiset<int, std::hash<int>, std::equal_to<int>, min_allocator<int>> C;
More information about the libcxx-commits
mailing list