[libc++] Remove initializer_list specific optimization in __tree

[philnik777]

We've seen in quite a few cases while optimizing __tree's copy construction that _DetachedTreeCache is actually quite slow and not necessarily an optimization at all. This patch removes the code, since it's now only used by operator=(initializer_list), which should be quite cold code. We might look into actually optimizing it again in the future, but I doubt an optimization will be small enough compared to the likely speedup in real-world code this would give.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

We've seen in quite a few cases while optimizing __tree's copy construction that _DetachedTreeCache is actually quite slow and not necessarily an optimization at all. This patch removes the code, since it's now only used by operator=(initializer_list), which should be quite cold code. We might look into actually optimizing it again in the future, but I doubt an optimization will be small enough compared to the likely speedup in real-world code this would give.

---

Full diff: https://github.com/llvm/llvm-project/pull/169413.diff

3 Files Affected:

• (modified) libcxx/include/__tree (-158)
• (modified) libcxx/include/map (+4-2)
• (modified) libcxx/include/set (+4-2)

diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index ceae22bb48702..f8064106de075 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -902,8 +902,6 @@ public:
   _LIBCPP_HIDE_FROM_ABI __tree& operator=(const __tree& __t);
   template <class _ForwardIterator>
   _LIBCPP_HIDE_FROM_ABI void __assign_unique(_ForwardIterator __first, _ForwardIterator __last);
-  template <class _InputIterator>
-  _LIBCPP_HIDE_FROM_ABI void __assign_multi(_InputIterator __first, _InputIterator __last);
   _LIBCPP_HIDE_FROM_ABI __tree(__tree&& __t) _NOEXCEPT_(
       is_nothrow_move_constructible<__node_allocator>::value&& is_nothrow_move_constructible<value_compare>::value);
   _LIBCPP_HIDE_FROM_ABI __tree(__tree&& __t, const allocator_type& __a);
@@ -1036,11 +1034,6 @@ public:
     }
   }
 
-  _LIBCPP_HIDE_FROM_ABI pair<iterator, bool> __node_assign_unique(const value_type& __v, __node_pointer __dest);
-
-  _LIBCPP_HIDE_FROM_ABI iterator __node_insert_multi(__node_pointer __nd);
-  _LIBCPP_HIDE_FROM_ABI iterator __node_insert_multi(const_iterator __p, __node_pointer __nd);
-
   template <class _InIter, class _Sent>
   _LIBCPP_HIDE_FROM_ABI void __insert_range_unique(_InIter __first, _Sent __last) {
     if (__first == __last)
@@ -1311,43 +1304,6 @@ private:
     __lhs = std::forward<_From>(__rhs);
   }
 
-  struct _DetachedTreeCache {
-    _LIBCPP_HIDE_FROM_ABI explicit _DetachedTreeCache(__tree* __t) _NOEXCEPT
-        : __t_(__t),
-          __cache_root_(__detach_from_tree(__t)) {
-      __advance();
-    }
-
-    _LIBCPP_HIDE_FROM_ABI __node_pointer __get() const _NOEXCEPT { return __cache_elem_; }
-
-    _LIBCPP_HIDE_FROM_ABI void __advance() _NOEXCEPT {
-      __cache_elem_ = __cache_root_;
-      if (__cache_root_) {
-        __cache_root_ = __detach_next(__cache_root_);
-      }
-    }
-
-    _LIBCPP_HIDE_FROM_ABI ~_DetachedTreeCache() {
-      __t_->destroy(__cache_elem_);
-      if (__cache_root_) {
-        while (__cache_root_->__parent_ != nullptr)
-          __cache_root_ = static_cast<__node_pointer>(__cache_root_->__parent_);
-        __t_->destroy(__cache_root_);
-      }
-    }
-
-    _DetachedTreeCache(_DetachedTreeCache const&)            = delete;
-    _DetachedTreeCache& operator=(_DetachedTreeCache const&) = delete;
-
-  private:
-    _LIBCPP_HIDE_FROM_ABI static __node_pointer __detach_from_tree(__tree* __t) _NOEXCEPT;
-    _LIBCPP_HIDE_FROM_ABI static __node_pointer __detach_next(__node_pointer) _NOEXCEPT;
-
-    __tree* __t_;
-    __node_pointer __cache_root_;
-    __node_pointer __cache_elem_;
-  };
-
   class __tree_deleter {
     __node_allocator& __alloc_;
 
@@ -1486,47 +1442,6 @@ private:
   }
 };
 
-// Precondition:  __size_ != 0
-template <class _Tp, class _Compare, class _Allocator>
-typename __tree<_Tp, _Compare, _Allocator>::__node_pointer
-__tree<_Tp, _Compare, _Allocator>::_DetachedTreeCache::__detach_from_tree(__tree* __t) _NOEXCEPT {
-  __node_pointer __cache                = static_cast<__node_pointer>(__t->__begin_node_);
-  __t->__begin_node_                    = __t->__end_node();
-  __t->__end_node()->__left_->__parent_ = nullptr;
-  __t->__end_node()->__left_            = nullptr;
-  __t->__size_                          = 0;
-  // __cache->__left_ == nullptr
-  if (__cache->__right_ != nullptr)
-    __cache = static_cast<__node_pointer>(__cache->__right_);
-  // __cache->__left_ == nullptr
-  // __cache->__right_ == nullptr
-  return __cache;
-}
-
-// Precondition:  __cache != nullptr
-//    __cache->left_ == nullptr
-//    __cache->right_ == nullptr
-//    This is no longer a red-black tree
-template <class _Tp, class _Compare, class _Allocator>
-typename __tree<_Tp, _Compare, _Allocator>::__node_pointer
-__tree<_Tp, _Compare, _Allocator>::_DetachedTreeCache::__detach_next(__node_pointer __cache) _NOEXCEPT {
-  if (__cache->__parent_ == nullptr)
-    return nullptr;
-  if (std::__tree_is_left_child(static_cast<__node_base_pointer>(__cache))) {
-    __cache->__parent_->__left_ = nullptr;
-    __cache                     = static_cast<__node_pointer>(__cache->__parent_);
-    if (__cache->__right_ == nullptr)
-      return __cache;
-    return static_cast<__node_pointer>(std::__tree_leaf(__cache->__right_));
-  }
-  // __cache is right child
-  __cache->__parent_unsafe()->__right_ = nullptr;
-  __cache                              = static_cast<__node_pointer>(__cache->__parent_);
-  if (__cache->__left_ == nullptr)
-    return __cache;
-  return static_cast<__node_pointer>(std::__tree_leaf(__cache->__left_));
-}
-
 template <class _Tp, class _Compare, class _Allocator>
 __tree<_Tp, _Compare, _Allocator>& __tree<_Tp, _Compare, _Allocator>::operator=(const __tree& __t) {
   if (this == std::addressof(__t))
@@ -1549,46 +1464,6 @@ __tree<_Tp, _Compare, _Allocator>& __tree<_Tp, _Compare, _Allocator>::operator=(
   return *this;
 }
 
-template <class _Tp, class _Compare, class _Allocator>
-template <class _ForwardIterator>
-void __tree<_Tp, _Compare, _Allocator>::__assign_unique(_ForwardIterator __first, _ForwardIterator __last) {
-  using _ITraits     = iterator_traits<_ForwardIterator>;
-  using _ItValueType = typename _ITraits::value_type;
-  static_assert(
-      is_same<_ItValueType, value_type>::value, "__assign_unique may only be called with the containers value type");
-  static_assert(
-      __has_forward_iterator_category<_ForwardIterator>::value, "__assign_unique requires a forward iterator");
-  if (__size_ != 0) {
-    _DetachedTreeCache __cache(this);
-    for (; __cache.__get() != nullptr && __first != __last; ++__first) {
-      if (__node_assign_unique(*__first, __cache.__get()).second)
-        __cache.__advance();
-    }
-  }
-  for (; __first != __last; ++__first)
-    __emplace_unique(*__first);
-}
-
-template <class _Tp, class _Compare, class _Allocator>
-template <class _InputIterator>
-void __tree<_Tp, _Compare, _Allocator>::__assign_multi(_InputIterator __first, _InputIterator __last) {
-  using _ITraits     = iterator_traits<_InputIterator>;
-  using _ItValueType = typename _ITraits::value_type;
-  static_assert(
-      is_same<_ItValueType, value_type>::value, "__assign_multi may only be called with the containers value_type");
-  if (__size_ != 0) {
-    _DetachedTreeCache __cache(this);
-    for (; __cache.__get() && __first != __last; ++__first) {
-      __assign_value(__cache.__get()->__get_value(), *__first);
-      __node_insert_multi(__cache.__get());
-      __cache.__advance();
-    }
-  }
-  const_iterator __e = end();
-  for (; __first != __last; ++__first)
-    __emplace_hint_multi(__e, *__first);
-}
-
 template <class _Tp, class _Compare, class _Allocator>
 __tree<_Tp, _Compare, _Allocator>::__tree(const __tree& __t)
     : __begin_node_(__end_node()),
@@ -1942,39 +1817,6 @@ __tree<_Tp, _Compare, _Allocator>::__emplace_hint_multi(const_iterator __p, _Arg
   return iterator(static_cast<__node_pointer>(__h.release()));
 }
 
-template <class _Tp, class _Compare, class _Allocator>
-pair<typename __tree<_Tp, _Compare, _Allocator>::iterator, bool>
-__tree<_Tp, _Compare, _Allocator>::__node_assign_unique(const value_type& __v, __node_pointer __nd) {
-  auto [__parent, __child] = __find_equal(__v);
-  __node_pointer __r       = static_cast<__node_pointer>(__child);
-  bool __inserted          = false;
-  if (__child == nullptr) {
-    __assign_value(__nd->__get_value(), __v);
-    __insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__nd));
-    __r        = __nd;
-    __inserted = true;
-  }
-  return pair<iterator, bool>(iterator(__r), __inserted);
-}
-
-template <class _Tp, class _Compare, class _Allocator>
-typename __tree<_Tp, _Compare, _Allocator>::iterator
-__tree<_Tp, _Compare, _Allocator>::__node_insert_multi(__node_pointer __nd) {
-  __end_node_pointer __parent;
-  __node_base_pointer& __child = __find_leaf_high(__parent, __nd->__get_value());
-  __insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__nd));
-  return iterator(__nd);
-}
-
-template <class _Tp, class _Compare, class _Allocator>
-typename __tree<_Tp, _Compare, _Allocator>::iterator
-__tree<_Tp, _Compare, _Allocator>::__node_insert_multi(const_iterator __p, __node_pointer __nd) {
-  __end_node_pointer __parent;
-  __node_base_pointer& __child = __find_leaf(__p, __parent, __nd->__get_value());
-  __insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__nd));
-  return iterator(__nd);
-}
-
 template <class _Tp, class _Compare, class _Allocator>
 typename __tree<_Tp, _Compare, _Allocator>::iterator
 __tree<_Tp, _Compare, _Allocator>::__remove_node_pointer(__node_pointer __ptr) _NOEXCEPT {
diff --git a/libcxx/include/map b/libcxx/include/map
index 0dca11cabd12e..e67f7cef5861d 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -1015,7 +1015,8 @@ public:
 #    endif
 
   _LIBCPP_HIDE_FROM_ABI map& operator=(initializer_list<value_type> __il) {
-    __tree_.__assign_unique(__il.begin(), __il.end());
+    clear();
+    insert(__il.begin(), __il.end());
     return *this;
   }
 
@@ -1689,7 +1690,8 @@ public:
 #    endif
 
   _LIBCPP_HIDE_FROM_ABI multimap& operator=(initializer_list<value_type> __il) {
-    __tree_.__assign_multi(__il.begin(), __il.end());
+    clear();
+    insert(__il.begin(), __il.end());
     return *this;
   }
 
diff --git a/libcxx/include/set b/libcxx/include/set
index 3d6f571a42a1a..f333d97defac1 100644
--- a/libcxx/include/set
+++ b/libcxx/include/set
@@ -692,7 +692,8 @@ public:
 #    endif
 
   _LIBCPP_HIDE_FROM_ABI set& operator=(initializer_list<value_type> __il) {
-    __tree_.__assign_unique(__il.begin(), __il.end());
+    clear();
+    insert(__il.begin(), __il.end());
     return *this;
   }
 
@@ -1136,7 +1137,8 @@ public:
 #    endif
 
   _LIBCPP_HIDE_FROM_ABI multiset& operator=(initializer_list<value_type> __il) {
-    __tree_.__assign_multi(__il.begin(), __il.end());
+    clear();
+    insert(__il.begin(), __il.end());
     return *this;
   }