[libc++] Optimize __hash_table copy constructors and assignment

[philnik777]

----------------------------------------------------------------------------------------------------------------------
Benchmark                                                                                             old          new
----------------------------------------------------------------------------------------------------------------------
std::unordered_set<int>::ctor(const&)/0                                                           15.4 ns      14.6 ns
std::unordered_set<int>::ctor(const&)/32                                                           686 ns       322 ns
std::unordered_set<int>::ctor(const&)/1024                                                       35839 ns     21490 ns
std::unordered_set<int>::ctor(const&)/8192                                                      385790 ns    280270 ns
std::unordered_set<int>::operator=(const&) (into cleared Container)/0                             15.1 ns      15.9 ns
std::unordered_set<int>::operator=(const&) (into cleared Container)/32                            1077 ns       333 ns
std::unordered_set<int>::operator=(const&) (into cleared Container)/1024                         31296 ns      9984 ns
std::unordered_set<int>::operator=(const&) (into cleared Container)/8192                        266776 ns    109418 ns
std::unordered_set<int>::operator=(const&) (into partially populated Container)/0                 15.1 ns      16.3 ns
std::unordered_set<int>::operator=(const&) (into partially populated Container)/32                 962 ns       320 ns
std::unordered_set<int>::operator=(const&) (into partially populated Container)/1024             31713 ns     10128 ns
std::unordered_set<int>::operator=(const&) (into partially populated Container)/8192            266113 ns    108525 ns
std::unordered_set<int>::operator=(const&) (into populated Container)/0                          0.990 ns      2.03 ns
std::unordered_set<int>::operator=(const&) (into populated Container)/32                           963 ns       263 ns
std::unordered_set<int>::operator=(const&) (into populated Container)/1024                       27600 ns      7793 ns
std::unordered_set<int>::operator=(const&) (into populated Container)/8192                      235295 ns     66248 ns
std::unordered_set<std::string>::ctor(const&)/0                                                   16.0 ns      15.0 ns
std::unordered_set<std::string>::ctor(const&)/32                                                  2950 ns      1277 ns
std::unordered_set<std::string>::ctor(const&)/1024                                              246935 ns     73762 ns
std::unordered_set<std::string>::ctor(const&)/8192                                             3310895 ns   2468608 ns
std::unordered_set<std::string>::operator=(const&) (into cleared Container)/0                     16.1 ns      15.8 ns
std::unordered_set<std::string>::operator=(const&) (into cleared Container)/32                    5856 ns      1039 ns
std::unordered_set<std::string>::operator=(const&) (into cleared Container)/1024                170436 ns     74836 ns
std::unordered_set<std::string>::operator=(const&) (into cleared Container)/8192               1574235 ns   1096891 ns
std::unordered_set<std::string>::operator=(const&) (into partially populated Container)/0         16.0 ns      16.3 ns
std::unordered_set<std::string>::operator=(const&) (into partially populated Container)/32        5571 ns      1064 ns
std::unordered_set<std::string>::operator=(const&) (into partially populated Container)/1024    199220 ns     75462 ns
std::unordered_set<std::string>::operator=(const&) (into partially populated Container)/8192   1552465 ns   1116094 ns
std::unordered_set<std::string>::operator=(const&) (into populated Container)/0                   1.70 ns      2.14 ns
std::unordered_set<std::string>::operator=(const&) (into populated Container)/32                  2562 ns       645 ns
std::unordered_set<std::string>::operator=(const&) (into populated Container)/1024              228608 ns     39100 ns
std::unordered_set<std::string>::operator=(const&) (into populated Container)/8192             2013723 ns    390401 ns

Fixes #77657

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Fixes #77657

---

Patch is 50.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/151951.diff

6 Files Affected:

• (modified) libcxx/include/__hash_table (+117-14)
• (modified) libcxx/include/unordered_map (+8-60)
• (modified) libcxx/include/unordered_set (+6-41)
• (modified) libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/assign_copy.pass.cpp (+269-107)
• (modified) libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/copy.pass.cpp (+97-118)
• (modified) libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/copy_alloc.pass.cpp (+71-109)

diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index dacc152030e14..e25d113dcc2b7 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -10,6 +10,7 @@
 #ifndef _LIBCPP___HASH_TABLE
 #define _LIBCPP___HASH_TABLE
 
+#include <__algorithm/fill_n.h>
 #include <__algorithm/max.h>
 #include <__algorithm/min.h>
 #include <__assert>
@@ -700,6 +701,38 @@ private:
 
   _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __size_; }
 
+  _LIBCPP_HIDE_FROM_ABI void
+  __copy_construct(__next_pointer __other_iter, __next_pointer __own_iter, size_t __current_chash) {
+    auto __bucket_count = bucket_count();
+
+    for (; __other_iter; __other_iter = __other_iter->__next_) {
+      __node_holder __new_node = __construct_node_hash(__other_iter->__hash(), __other_iter->__upcast()->__get_value());
+
+      size_t __new_chash = std::__constrain_hash(__new_node->__hash(), __bucket_count);
+      if (__new_chash != __current_chash) {
+        __bucket_list_[__new_chash] = __own_iter;
+        __current_chash             = __new_chash;
+      }
+
+      __own_iter->__next_ = static_cast<__next_pointer>(__new_node.release());
+      __own_iter          = __own_iter->__next_;
+    }
+  }
+
+  _LIBCPP_HIDE_FROM_ABI void __copy_construct(__next_pointer __other_iter) {
+    __next_pointer __own_iter = __first_node_.__ptr();
+    {
+      __node_holder __new_node = __construct_node_hash(__other_iter->__hash(), __other_iter->__upcast()->__get_value());
+      __own_iter->__next_      = static_cast<__next_pointer>(__new_node.release());
+    }
+
+    size_t __current_chash          = std::__constrain_hash(__own_iter->__next_->__hash(), bucket_count());
+    __bucket_list_[__current_chash] = __own_iter;
+    __other_iter                    = __other_iter->__next_;
+    __own_iter                      = __own_iter->__next_;
+    __copy_construct(__other_iter, __own_iter, __current_chash);
+  }
+
 public:
   _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __size_; }
 
@@ -1048,16 +1081,29 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const allocator_type& __a
       __max_load_factor_(1.0f) {}
 
 template <class _Tp, class _Hash, class _Equal, class _Alloc>
-__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u)
+__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __other)
     : __bucket_list_(nullptr,
-                     __bucket_list_deleter(allocator_traits<__pointer_allocator>::select_on_container_copy_construction(
-                                               __u.__bucket_list_.get_deleter().__alloc()),
+                     __bucket_list_deleter(__pointer_alloc_traits::select_on_container_copy_construction(
+                                               __other.__bucket_list_.get_deleter().__alloc()),
                                            0)),
-      __node_alloc_(allocator_traits<__node_allocator>::select_on_container_copy_construction(__u.__node_alloc())),
+      __node_alloc_(__node_traits::select_on_container_copy_construction(__other.__node_alloc())),
       __size_(0),
-      __hasher_(__u.hash_function()),
-      __max_load_factor_(__u.__max_load_factor_),
-      __key_eq_(__u.__key_eq_) {}
+      __hasher_(__other.hash_function()),
+      __max_load_factor_(__other.__max_load_factor_),
+      __key_eq_(__other.__key_eq_) {
+  if (__other.size() == 0)
+    return;
+
+  auto& __bucket_list_del = __bucket_list_.get_deleter();
+  auto __bucket_count = __other.bucket_count();
+  __bucket_list_.reset(__pointer_alloc_traits::allocate(__bucket_list_del.__alloc(), __bucket_count));
+  __bucket_list_del.size() = __bucket_count;
+
+  std::fill_n(__bucket_list_.get(), __bucket_count, nullptr);
+
+  __copy_construct(__other.__first_node_.__next_);
+  __size_ = __other.size();
+}
 
 template <class _Tp, class _Hash, class _Equal, class _Alloc>
 __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u, const allocator_type& __a)
@@ -1131,14 +1177,71 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__copy_assign_alloc(const __hash_
 }
 
 template <class _Tp, class _Hash, class _Equal, class _Alloc>
-__hash_table<_Tp, _Hash, _Equal, _Alloc>& __hash_table<_Tp, _Hash, _Equal, _Alloc>::operator=(const __hash_table& __u) {
-  if (this != std::addressof(__u)) {
-    __copy_assign_alloc(__u);
-    hash_function()   = __u.hash_function();
-    key_eq()          = __u.key_eq();
-    max_load_factor() = __u.max_load_factor();
-    __assign_multi(__u.begin(), __u.end());
+__hash_table<_Tp, _Hash, _Equal, _Alloc>&
+__hash_table<_Tp, _Hash, _Equal, _Alloc>::operator=(const __hash_table& __other) {
+  if (this == std::addressof(__other))
+    return *this;
+
+  __copy_assign_alloc(__other);
+  hash_function()   = __other.hash_function();
+  key_eq()          = __other.key_eq();
+  max_load_factor() = __other.max_load_factor();
+
+  if (__other.size() == 0) {
+    clear();
+    return *this;
+  }
+
+  auto __bucket_count = __other.bucket_count();
+  if (__bucket_count != bucket_count()) {
+    auto& __bucket_list_del = __bucket_list_.get_deleter();
+    __bucket_list_.reset(__pointer_alloc_traits::allocate(__bucket_list_del.__alloc(), __bucket_count));
+    __bucket_list_del.size() = __bucket_count;
+  }
+  std::fill_n(__bucket_list_.get(), __bucket_count, nullptr);
+
+  if (!__first_node_.__next_) {
+    __copy_construct(__other.__first_node_.__next_);
+    __size_ = __other.size();
+    return *this;
   }
+
+  __next_pointer __other_iter = __other.__first_node_.__next_;
+  __next_pointer __own_iter = __first_node_.__ptr();
+  {
+    __node_pointer __next = __own_iter->__next_->__upcast();
+    __assign_value(__next->__get_value(), __other_iter->__upcast()->__get_value());
+    __next->__hash_ = __other_iter->__hash();
+  }
+  size_t __current_chash          = std::__constrain_hash(__own_iter->__next_->__hash(), __bucket_count);
+  __bucket_list_[__current_chash] = __own_iter;
+  __other_iter = __other_iter->__next_;
+  __own_iter = __own_iter->__next_;
+
+  while (__other_iter && __own_iter->__next_) {
+    __node_pointer __next = __own_iter->__next_->__upcast();
+    __assign_value(__next->__get_value(), __other_iter->__upcast()->__get_value());
+    __next->__hash_ = __other_iter->__hash();
+
+    size_t __new_chash = std::__constrain_hash(__next->__hash_, __bucket_count);
+    if (__new_chash != __current_chash) {
+      __bucket_list_[__new_chash] = __own_iter;
+      __current_chash             = __new_chash;
+    }
+
+    __other_iter = __other_iter->__next_;
+    __own_iter   = __own_iter->__next_;
+  }
+
+  if (__own_iter->__next_) {
+    __deallocate_node(__own_iter->__next_);
+    __own_iter->__next_ = nullptr;
+  } else {
+    __copy_construct(__other_iter, __own_iter, __current_chash);
+  }
+
+  __size_ = __other.size();
+
   return *this;
 }
 
diff --git a/libcxx/include/unordered_map b/libcxx/include/unordered_map
index 97c2c52eba337..009dfb7a1e2b7 100644
--- a/libcxx/include/unordered_map
+++ b/libcxx/include/unordered_map
@@ -1046,10 +1046,11 @@ public:
 #  endif
 
   _LIBCPP_HIDE_FROM_ABI explicit unordered_map(const allocator_type& __a);
-  _LIBCPP_HIDE_FROM_ABI unordered_map(const unordered_map& __u);
+  _LIBCPP_HIDE_FROM_ABI unordered_map(const unordered_map& __u) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_map(const unordered_map& __u, const allocator_type& __a);
 #  ifndef _LIBCPP_CXX03_LANG
-  _LIBCPP_HIDE_FROM_ABI unordered_map(unordered_map&& __u) _NOEXCEPT_(is_nothrow_move_constructible<__table>::value);
+  _LIBCPP_HIDE_FROM_ABI unordered_map(unordered_map&& __u)
+      _NOEXCEPT_(is_nothrow_move_constructible<__table>::value) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_map(unordered_map&& __u, const allocator_type& __a);
   _LIBCPP_HIDE_FROM_ABI unordered_map(initializer_list<value_type> __il);
   _LIBCPP_HIDE_FROM_ABI
@@ -1099,24 +1100,10 @@ public:
     static_assert(sizeof(std::__diagnose_unordered_container_requirements<_Key, _Hash, _Pred>(0)), "");
   }
 
-  _LIBCPP_HIDE_FROM_ABI unordered_map& operator=(const unordered_map& __u) {
-#  ifndef _LIBCPP_CXX03_LANG
-    __table_ = __u.__table_;
-#  else
-    if (this != std::addressof(__u)) {
-      __table_.clear();
-      __table_.hash_function()   = __u.__table_.hash_function();
-      __table_.key_eq()          = __u.__table_.key_eq();
-      __table_.max_load_factor() = __u.__table_.max_load_factor();
-      __table_.__copy_assign_alloc(__u.__table_);
-      insert(__u.begin(), __u.end());
-    }
-#  endif
-    return *this;
-  }
+  _LIBCPP_HIDE_FROM_ABI unordered_map& operator=(const unordered_map& __u) = default;
 #  ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_HIDE_FROM_ABI unordered_map& operator=(unordered_map&& __u)
-      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value);
+      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_map& operator=(initializer_list<value_type> __il);
 #  endif // _LIBCPP_CXX03_LANG
 
@@ -1563,12 +1550,6 @@ unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(
   insert(__first, __last);
 }
 
-template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
-unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(const unordered_map& __u) : __table_(__u.__table_) {
-  __table_.__rehash_unique(__u.bucket_count());
-  insert(__u.begin(), __u.end());
-}
-
 template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(const unordered_map& __u, const allocator_type& __a)
     : __table_(__u.__table_, typename __table::allocator_type(__a)) {
@@ -1578,11 +1559,6 @@ unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(const unordered_ma
 
 #  ifndef _LIBCPP_CXX03_LANG
 
-template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
-inline unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(unordered_map&& __u)
-    _NOEXCEPT_(is_nothrow_move_constructible<__table>::value)
-    : __table_(std::move(__u.__table_)) {}
-
 template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(unordered_map&& __u, const allocator_type& __a)
     : __table_(std::move(__u.__table_), typename __table::allocator_type(__a)) {
@@ -1618,14 +1594,6 @@ unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_map(
   insert(__il.begin(), __il.end());
 }
 
-template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
-inline unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>&
-unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::operator=(unordered_map&& __u)
-    _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) {
-  __table_ = std::move(__u.__table_);
-  return *this;
-}
-
 template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 inline unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>&
 unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::operator=(initializer_list<value_type> __il) {
@@ -1852,11 +1820,11 @@ public:
 #  endif
 
   _LIBCPP_HIDE_FROM_ABI explicit unordered_multimap(const allocator_type& __a);
-  _LIBCPP_HIDE_FROM_ABI unordered_multimap(const unordered_multimap& __u);
+  _LIBCPP_HIDE_FROM_ABI unordered_multimap(const unordered_multimap& __u) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_multimap(const unordered_multimap& __u, const allocator_type& __a);
 #  ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_HIDE_FROM_ABI unordered_multimap(unordered_multimap&& __u)
-      _NOEXCEPT_(is_nothrow_move_constructible<__table>::value);
+      _NOEXCEPT_(is_nothrow_move_constructible<__table>::value) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_multimap(unordered_multimap&& __u, const allocator_type& __a);
   _LIBCPP_HIDE_FROM_ABI unordered_multimap(initializer_list<value_type> __il);
   _LIBCPP_HIDE_FROM_ABI unordered_multimap(
@@ -1923,7 +1891,7 @@ public:
   }
 #  ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_HIDE_FROM_ABI unordered_multimap& operator=(unordered_multimap&& __u)
-      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value);
+      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_multimap& operator=(initializer_list<value_type> __il);
 #  endif // _LIBCPP_CXX03_LANG
 
@@ -2315,13 +2283,6 @@ template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 inline unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_multimap(const allocator_type& __a)
     : __table_(typename __table::allocator_type(__a)) {}
 
-template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
-unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_multimap(const unordered_multimap& __u)
-    : __table_(__u.__table_) {
-  __table_.__rehash_multi(__u.bucket_count());
-  insert(__u.begin(), __u.end());
-}
-
 template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_multimap(
     const unordered_multimap& __u, const allocator_type& __a)
@@ -2332,11 +2293,6 @@ unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_multimap(
 
 #  ifndef _LIBCPP_CXX03_LANG
 
-template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
-inline unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_multimap(unordered_multimap&& __u)
-    _NOEXCEPT_(is_nothrow_move_constructible<__table>::value)
-    : __table_(std::move(__u.__table_)) {}
-
 template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_multimap(
     unordered_multimap&& __u, const allocator_type& __a)
@@ -2373,14 +2329,6 @@ unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::unordered_multimap(
   insert(__il.begin(), __il.end());
 }
 
-template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
-inline unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>&
-unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::operator=(unordered_multimap&& __u)
-    _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) {
-  __table_ = std::move(__u.__table_);
-  return *this;
-}
-
 template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
 inline unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>&
 unordered_multimap<_Key, _Tp, _Hash, _Pred, _Alloc>::operator=(initializer_list<value_type> __il) {
diff --git a/libcxx/include/unordered_set b/libcxx/include/unordered_set
index 475715db62bdb..09bd81a22eae5 100644
--- a/libcxx/include/unordered_set
+++ b/libcxx/include/unordered_set
@@ -703,7 +703,7 @@ public:
 #  endif
 
   _LIBCPP_HIDE_FROM_ABI explicit unordered_set(const allocator_type& __a);
-  _LIBCPP_HIDE_FROM_ABI unordered_set(const unordered_set& __u);
+  _LIBCPP_HIDE_FROM_ABI unordered_set(const unordered_set& __u) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_set(const unordered_set& __u, const allocator_type& __a);
 #  ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_HIDE_FROM_ABI unordered_set(unordered_set&& __u) _NOEXCEPT_(is_nothrow_move_constructible<__table>::value);
@@ -733,13 +733,10 @@ public:
     static_assert(sizeof(std::__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
   }
 
-  _LIBCPP_HIDE_FROM_ABI unordered_set& operator=(const unordered_set& __u) {
-    __table_ = __u.__table_;
-    return *this;
-  }
+  _LIBCPP_HIDE_FROM_ABI unordered_set& operator=(const unordered_set& __u) = default;
 #  ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_HIDE_FROM_ABI unordered_set& operator=(unordered_set&& __u)
-      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value);
+      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_set& operator=(initializer_list<value_type> __il);
 #  endif // _LIBCPP_CXX03_LANG
 
@@ -1070,12 +1067,6 @@ unordered_set<_Value, _Hash, _Pred, _Alloc>::unordered_set(
 template <class _Value, class _Hash, class _Pred, class _Alloc>
 inline unordered_set<_Value, _Hash, _Pred, _Alloc>::unordered_set(const allocator_type& __a) : __table_(__a) {}
 
-template <class _Value, class _Hash, class _Pred, class _Alloc>
-unordered_set<_Value, _Hash, _Pred, _Alloc>::unordered_set(const unordered_set& __u) : __table_(__u.__table_) {
-  __table_.__rehash_unique(__u.bucket_count());
-  insert(__u.begin(), __u.end());
-}
-
 template <class _Value, class _Hash, class _Pred, class _Alloc>
 unordered_set<_Value, _Hash, _Pred, _Alloc>::unordered_set(const unordered_set& __u, const allocator_type& __a)
     : __table_(__u.__table_, __a) {
@@ -1125,14 +1116,6 @@ unordered_set<_Value, _Hash, _Pred, _Alloc>::unordered_set(
   insert(__il.begin(), __il.end());
 }
 
-template <class _Value, class _Hash, class _Pred, class _Alloc>
-inline unordered_set<_Value, _Hash, _Pred, _Alloc>&
-unordered_set<_Value, _Hash, _Pred, _Alloc>::operator=(unordered_set&& __u)
-    _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) {
-  __table_ = std::move(__u.__table_);
-  return *this;
-}
-
 template <class _Value, class _Hash, class _Pred, class _Alloc>
 inline unordered_set<_Value, _Hash, _Pred, _Alloc>&
 unordered_set<_Value, _Hash, _Pred, _Alloc>::operator=(initializer_list<value_type> __il) {
@@ -1308,7 +1291,7 @@ public:
 #  endif
 
   _LIBCPP_HIDE_FROM_ABI explicit unordered_multiset(const allocator_type& __a);
-  _LIBCPP_HIDE_FROM_ABI unordered_multiset(const unordered_multiset& __u);
+  _LIBCPP_HIDE_FROM_ABI unordered_multiset(const unordered_multiset& __u) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_multiset(const unordered_multiset& __u, const allocator_type& __a);
 #  ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_HIDE_FROM_ABI unordered_multiset(unordered_multiset&& __u)
@@ -1339,13 +1322,10 @@ public:
     static_assert(sizeof(std::__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
   }
 
-  _LIBCPP_HIDE_FROM_ABI unordered_multiset& operator=(const unordered_multiset& __u) {
-    __table_ = __u.__table_;
-    return *this;
-  }
+  _LIBCPP_HIDE_FROM_ABI unordered_multiset& operator=(const unordered_multiset& __u) = default;
 #  ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_HIDE_FROM_ABI unordered_multiset& operator=(unordered_multiset&& __u)
-      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value);
+      _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) = default;
   _LIBCPP_HIDE_FROM_ABI unordered_multiset& operator=(initializer_list<value_type> __il);
 #  endif // _LIBCPP_CXX03_LANG
 
@@ -1685,13 +1665,6 @@ template <class _Value, class _Hash, class _Pred, class _Alloc>
 inline unordered_multiset<_Value, _Hash, _Pred, _Alloc>::unordered_multiset(const allocator_type& __a)
     : __table_(__a) {}
 
-template <class _Value, class _Hash, class _Pred, class _Alloc>
-unordered_multiset<_Value, _Hash, _Pred, _Alloc>::unordered_multiset(const unordered_multiset& __u)
-    : __table_(__u.__table_) {
-  __table_.__rehash_multi(__u.bucket_count());
-  insert(__u.begin(), __u.end());
-}
-
 template <class _Value, class _Hash, class _Pred, class _Alloc>
 unordered_multiset<_Value, _Hash, _Pred, _Alloc>::unordered_multiset(
     const unordered_multiset& __u, const allocator_type& __a)
@@ -1743,14 +1716,6 @@ unordered_multiset<_Value, _Hash, _Pred, _Alloc>::unordered_multiset(
   insert(__il.begin(), __il.end());
 }
 
-template <class _Value, class _Hash, class _Pred, class _Alloc>
-inline unordered_multiset<_Value, _Hash, _Pred, _Alloc>&
-unordered_multiset<_Value, _Hash, _Pred, _Alloc>::operator=(unordered_multiset&& __u)
-    _NOEXCEPT_(is_nothrow_move_assignable<__table>::value) {
-  __table_ = std::move(__u.__table_);
-  return *this;
-}
-
 template <class _Value, class _Hash, class _Pred, class _Alloc>
 inline unordered_multiset<_Value, _Hash, _Pred, _Alloc>&
 unordered_multiset<_Value, _Hash, _Pred, _Alloc>::operator=(initializer_list<value_type> __il) {
diff --git a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/assign_copy.pass.cpp b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/assign_copy.pass.cpp
index 34dec07b03e08..5c868bba2ccbd 100644
--- a/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/assign_copy.pass.cpp
+++ b/libcxx/test/std/containers/unord/unord.map/unord.map.cnstr/assign_copy.pass.cpp
@@ -15,12 +15,13 @@
 // unordered_map& operator=(const unordered_map& u);
 
 #include <algorithm>
-#include <unordered_map>
-#include <string>
 #include <cassert>
 #include <cfloat>
 #include <cmath>
 #i...
[truncated]

[ldionne]

LGTM, let's do something similar for set and multi_{set,map}'s tests!

[nico]

This one also changes behavior, right? I think before this change, the copy of an unordered_foo had the same iteration order as the original unordered_foo, and after this change it's no longer true.

(This is fine by the standard, but still nice to mention in release notes.)

For example, this program used to print

url_1
url_2

but after this change it prints

url_2
url_1

:

% cat repro.cc 
#include <unordered_map>
#include <string>
#include <vector>

struct Context {
  explicit Context(std::string session_id) : session_id(session_id) {}
  Context(const Context&) = default;

  std::string session_id;
  std::unordered_map<std::string, std::vector<std::string>> url_passages_map;
};

void f(Context c) {
  for (auto const& u : c.url_passages_map)
    printf("%s\n", u.first.c_str());
}

int main() {
  Context context("1");
  context.url_passages_map.insert({"url_1", {"passage_11", "passage_12"}});
  context.url_passages_map.insert({"url_2", {"passage_21", "passage_22"}});
  f(context); // Copies.
}

% clang++  repro.cc -std=c++20 -isystem third_party/libc++/src/include -isystem buildtools/third_party/libc++ -D_LIBCPP_HARDENING_MODE_DEFAULT=_LIBCPP_HARDENING_MODE_NONE -nostdinc++ out/gn/libc++_chrome.dylib
% DYLD_LIBRARY_PATH=$PWD/out/gn ./a.out
url_2
url_1

[philnik777]

This one also changes behavior, right?

Yes.

I think before this change, the copy of an unordered_foo had the same iteration order as the original unordered_foo, and after this change it's no longer true.

It's actually the other way around: https://godbolt.org/z/z6MzPYd41

(This is fine by the standard, but still nice to mention in release notes.)

I'm not against it, but I'm also not sure it's worth a note. We're basically changing from "the elements could be in any order" to "they're the same before and after the copy".

[nico]

I'm not against it, but I'm also not sure it's worth a note. We're basically changing from "the elements could be in any order" to "they're the same before and after the copy".

I'd find it useful. IMHO the main point of release notes is to advertise changes in behavior :)

…but we don't update to releases in chromium, so I'd personally find it even more useful if an optimization that changes behavior said so in the commit message. Just "Optimize X" imho carries an implicit "doesn't change behavior, just makes faster". When scanning lots of commit messages to figure out why various tests broke, it's helpful to have good commit messages. Presumably, most people do use the releases though, and would appreciate the release notes entry.

[philnik777]

I'm not against it, but I'm also not sure it's worth a note. We're basically changing from "the elements could be in any order" to "they're the same before and after the copy".

I'd find it useful. IMHO the main point of release notes is to advertise changes in behavior :)

…but we don't update to releases in chromium, so I'd personally find it even more useful if an optimization that changes behavior said so in the commit message. Just "Optimize X" imho carries an implicit "doesn't change behavior, just makes faster".

At least for libc++ patches I don't think that can be assumed. Almost all optimizations change the observable behaviour in some way. Going through the optimizations in this release so far:

• (multi)map/set copy assignment/construction: The order in which the elements are copied is changed and all comparator calls are removed
• unordered(_multi)_map/set: The number of hash calls almost certainly changed, and the order of the elements changed
• map/set::erase and ::find: The number of comparison calls changed
• Avoiding allocations in path::lexically_relative: The number of calls to ::operator new changed
• multimap/set::insert(iterator, iterator): I think this is one of the few that may actually not be observable
• unordered_map/set::erase: The number of calls to the hasher changed

I don't think we want to add a "potentially breaking" release note for every one of them, but I also don't really know where the line is. I feel like this patch is on the edge. The other patch you commented on was a rather clear "this is going to break users", since it's potentially changing the return value. Here I don't know how much of an impact this really has. Users of an unordered container should expect their elements to indeed be unordered, but it's also very much conceivable that people have golden tests that get broken by this. I don't really expect this to have an impact on non-test code though. Does that warrant a "potentially breaking" note? IDK.

When scanning lots of commit messages to figure out why various tests broke, it's helpful to have good commit messages. Presumably, most people do use the releases though, and would appreciate the release notes entry.

[nico]

That's a convincing point, thanks :)

Just as a data point, we've now rolled both changes into chromium (the find() one only a few hours ago, so not yet clear if there will be reports of things breaking that weren't covered by tests – I did try to go through all callers of find() and most did the right thing). The find() change broke code (well, exposed broken code) in just one file, but it was production code. This change here did break code in three files, but it was indeed all test code. (I don't have point here; just sharing in case it's interesting.)

[asmok-g]

@philnik777 Heads-up:
We noticed this is causing errors when using __gnu_cxx::hash_multiset, __gnu_cxx::hash_multimap where the resulting container has duplicate entries. working on a repro.

[alexfh]

@philnik777 Heads-up: We noticed this is causing errors when using __gnu_cxx::hash_multiset, __gnu_cxx::hash_multimap where the resulting container has duplicate entries. working on a repro.

It looks like the commit changed the common base of std::unordered_map / std::unordered_set (libcxx/include/__hash_table), but didn't update the __gnu_cxx containers (implemented in libcxx/include/ext/hash_map and libcxx/include/ext/hash_set) accordingly.

[alexfh]

@philnik777 could you update the __gnu_cxx hash container implementations as well?

[asmok-g]

Here's a repro: https://godbolt.org/z/eM99Tva1f. I'm sending a PR

[alexfh]

@philnik777 landed fixes in #160043 and #160466. It would have been nice to mention this here though.

And there's an open question whether hash_map and hash_set also need similar fixes. They can't contain multiple identical keys, but IIUC they are repeating all the insertions done in the base class' constructor in their own constructor, effectively being ~2x slower. So something like #160525 seems to be needed.