-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Reapply "[libc++] Optimize vector growing of trivially relocatable types" #80558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reapply "[libc++] Optimize vector growing of trivially relocatable types" #80558
Conversation
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis reapplies #76657. Non-trivial elements didn't get destroyed previously. This fixes the bug and adds tests for all the vector insertion functions. Patch is 29.05 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80558.diff 14 Files Affected:
diff --git a/libcxx/benchmarks/ContainerBenchmarks.h b/libcxx/benchmarks/ContainerBenchmarks.h
index 9a9abfd3b0d0f..744505b439985 100644
--- a/libcxx/benchmarks/ContainerBenchmarks.h
+++ b/libcxx/benchmarks/ContainerBenchmarks.h
@@ -80,7 +80,7 @@ void BM_ConstructFromRange(benchmark::State& st, Container, GenInputs gen) {
}
template <class Container>
-void BM_Pushback(benchmark::State& state, Container c) {
+void BM_Pushback_no_grow(benchmark::State& state, Container c) {
int count = state.range(0);
c.reserve(count);
while (state.KeepRunningBatch(count)) {
diff --git a/libcxx/benchmarks/vector_operations.bench.cpp b/libcxx/benchmarks/vector_operations.bench.cpp
index 38b14c56756fb..da21d1806c012 100644
--- a/libcxx/benchmarks/vector_operations.bench.cpp
+++ b/libcxx/benchmarks/vector_operations.bench.cpp
@@ -1,6 +1,7 @@
#include <cstdint>
#include <cstdlib>
#include <cstring>
+#include <deque>
#include <functional>
#include <vector>
@@ -39,6 +40,21 @@ BENCHMARK_CAPTURE(BM_ConstructFromRange, vector_size_t, std::vector<size_t>{}, g
BENCHMARK_CAPTURE(BM_ConstructFromRange, vector_string, std::vector<std::string>{}, getRandomStringInputs)
->Arg(TestNumInputs);
-BENCHMARK_CAPTURE(BM_Pushback, vector_int, std::vector<int>{})->Arg(TestNumInputs);
+BENCHMARK_CAPTURE(BM_Pushback_no_grow, vector_int, std::vector<int>{})->Arg(TestNumInputs);
+
+template <class T>
+void bm_grow(benchmark::State& state) {
+ for (auto _ : state) {
+ std::vector<T> vec;
+ benchmark::DoNotOptimize(vec);
+ for (size_t i = 0; i != 2048; ++i)
+ vec.emplace_back();
+ benchmark::DoNotOptimize(vec);
+ }
+}
+BENCHMARK(bm_grow<int>);
+BENCHMARK(bm_grow<std::string>);
+BENCHMARK(bm_grow<std::unique_ptr<int>>);
+BENCHMARK(bm_grow<std::deque<int>>);
BENCHMARK_MAIN();
diff --git a/libcxx/docs/ReleaseNotes/19.rst b/libcxx/docs/ReleaseNotes/19.rst
index db731de2e4399..908d46b711a5a 100644
--- a/libcxx/docs/ReleaseNotes/19.rst
+++ b/libcxx/docs/ReleaseNotes/19.rst
@@ -44,8 +44,8 @@ Implemented Papers
Improvements and New Features
-----------------------------
-TODO
+- The performance of growing ``std::vector`` has been improved for trivially relocatable types.
Deprecations and Removals
-------------------------
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 6a845c813dc33..d8ee598dc2633 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -808,6 +808,7 @@ set(files
__type_traits/is_trivially_lexicographically_comparable.h
__type_traits/is_trivially_move_assignable.h
__type_traits/is_trivially_move_constructible.h
+ __type_traits/is_trivially_relocatable.h
__type_traits/is_unbounded_array.h
__type_traits/is_union.h
__type_traits/is_unsigned.h
diff --git a/libcxx/include/__memory/uninitialized_algorithms.h b/libcxx/include/__memory/uninitialized_algorithms.h
index 9aff93a896486..9733bb748f665 100644
--- a/libcxx/include/__memory/uninitialized_algorithms.h
+++ b/libcxx/include/__memory/uninitialized_algorithms.h
@@ -29,6 +29,7 @@
#include <__type_traits/is_trivially_copy_constructible.h>
#include <__type_traits/is_trivially_move_assignable.h>
#include <__type_traits/is_trivially_move_constructible.h>
+#include <__type_traits/is_trivially_relocatable.h>
#include <__type_traits/is_unbounded_array.h>
#include <__type_traits/negation.h>
#include <__type_traits/remove_const.h>
@@ -594,60 +595,57 @@ __uninitialized_allocator_copy(_Alloc& __alloc, _Iter1 __first1, _Sent1 __last1,
return std::__rewrap_iter(__first2, __result);
}
-// Move-construct the elements [__first1, __last1) into [__first2, __first2 + N)
-// if the move constructor is noexcept, where N is distance(__first1, __last1).
-//
-// Otherwise try to copy all elements. If an exception is thrown the already copied
-// elements are destroyed in reverse order of their construction.
-template <class _Alloc, class _Iter1, class _Sent1, class _Iter2>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Iter2
-__uninitialized_allocator_move_if_noexcept(_Alloc& __alloc, _Iter1 __first1, _Sent1 __last1, _Iter2 __first2) {
- static_assert(__is_cpp17_move_insertable<_Alloc>::value,
- "The specified type does not meet the requirements of Cpp17MoveInsertable");
- auto __destruct_first = __first2;
- auto __guard =
- std::__make_exception_guard(_AllocatorDestroyRangeReverse<_Alloc, _Iter2>(__alloc, __destruct_first, __first2));
- while (__first1 != __last1) {
-#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
- allocator_traits<_Alloc>::construct(__alloc, std::__to_address(__first2), std::move_if_noexcept(*__first1));
-#else
- allocator_traits<_Alloc>::construct(__alloc, std::__to_address(__first2), std::move(*__first1));
-#endif
- ++__first1;
- ++__first2;
- }
- __guard.__complete();
- return __first2;
-}
-
template <class _Alloc, class _Type>
struct __allocator_has_trivial_move_construct : _Not<__has_construct<_Alloc, _Type*, _Type&&> > {};
template <class _Type>
struct __allocator_has_trivial_move_construct<allocator<_Type>, _Type> : true_type {};
-#ifndef _LIBCPP_COMPILER_GCC
-template <
- class _Alloc,
- class _Iter1,
- class _Iter2,
- class _Type = typename iterator_traits<_Iter1>::value_type,
- class = __enable_if_t<is_trivially_move_constructible<_Type>::value && is_trivially_move_assignable<_Type>::value &&
- __allocator_has_trivial_move_construct<_Alloc, _Type>::value> >
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Iter2
-__uninitialized_allocator_move_if_noexcept(_Alloc&, _Iter1 __first1, _Iter1 __last1, _Iter2 __first2) {
- if (__libcpp_is_constant_evaluated()) {
- while (__first1 != __last1) {
- std::__construct_at(std::__to_address(__first2), std::move(*__first1));
- ++__first1;
- ++__first2;
+template <class _Alloc, class _Tp>
+struct __allocator_has_trivial_destroy : _Not<__has_destroy<_Alloc, _Tp*> > {};
+
+template <class _Tp, class _Up>
+struct __allocator_has_trivial_destroy<allocator<_Tp>, _Up> : true_type {};
+
+// __uninitialized_allocator_relocate relocates the objects in [__first, __last) into __result.
+// Relocation means that the objects in [__first, __last) are placed into __result as-if by move-construct and destroy,
+// except that the move constructor and destructor may never be called if they are known to be equivalent to a memcpy.
+//
+// Preconditions: __result doesn't contain any objects and [__first, __last) contains objects
+// Postconditions: __result contains the objects from [__first, __last) and
+// [__first, __last) doesn't contain any objects
+//
+// The strong exception guarantee is provided if any of the following are true:
+// - is_nothrow_move_constructible<_Tp>
+// - is_copy_constructible<_Tp>
+// - __libcpp_is_trivially_relocatable<_Tp>
+template <class _Alloc, class _Tp>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
+__uninitialized_allocator_relocate(_Alloc& __alloc, _Tp* __first, _Tp* __last, _Tp* __result) {
+ static_assert(__is_cpp17_move_insertable<_Alloc>::value,
+ "The specified type does not meet the requirements of Cpp17MoveInsertable");
+ if (__libcpp_is_constant_evaluated() || !__libcpp_is_trivially_relocatable<_Tp>::value ||
+ !__allocator_has_trivial_move_construct<_Alloc, _Tp>::value ||
+ !__allocator_has_trivial_destroy<_Alloc, _Tp>::value) {
+ auto __destruct_first = __result;
+ auto __guard =
+ std::__make_exception_guard(_AllocatorDestroyRangeReverse<_Alloc, _Tp*>(__alloc, __destruct_first, __result));
+ auto __iter = __first;
+ while (__iter != __last) {
+#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+ allocator_traits<_Alloc>::construct(__alloc, __result, std::move_if_noexcept(*__iter));
+#else
+ allocator_traits<_Alloc>::construct(__alloc, __result, std::move(*__iter));
+#endif
+ ++__iter;
+ ++__result;
}
- return __first2;
+ __guard.__complete();
+ std::__allocator_destroy(__alloc, __first, __last);
} else {
- return std::move(__first1, __last1, __first2);
+ __builtin_memcpy(__result, __first, sizeof(_Tp) * (__last - __first));
}
}
-#endif // _LIBCPP_COMPILER_GCC
_LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index db473eaa50a6b..a505dab8dd749 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -21,6 +21,7 @@
#include <__memory/compressed_pair.h>
#include <__type_traits/add_lvalue_reference.h>
#include <__type_traits/common_type.h>
+#include <__type_traits/conditional.h>
#include <__type_traits/dependent_type.h>
#include <__type_traits/integral_constant.h>
#include <__type_traits/is_array.h>
@@ -33,6 +34,7 @@
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
+#include <__type_traits/is_trivially_relocatable.h>
#include <__type_traits/is_void.h>
#include <__type_traits/remove_extent.h>
#include <__type_traits/type_identity.h>
@@ -129,6 +131,17 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
static_assert(!is_rvalue_reference<deleter_type>::value, "the specified deleter type cannot be an rvalue reference");
+ // A unique_ptr contains the following members which may be trivially relocatable:
+ // - pointer : this may be trivially relocatable, so it's checked
+ // - deleter_type: this may be trivially relocatable, so it's checked
+ //
+ // This unique_ptr implementation only contains a pointer to the unique object and a deleter, so there are no
+ // references to itself. This means that the entire structure is trivially relocatable if its members are.
+ using __trivially_relocatable = __conditional_t<
+ __libcpp_is_trivially_relocatable<pointer>::value && __libcpp_is_trivially_relocatable<deleter_type>::value,
+ unique_ptr,
+ void>;
+
private:
__compressed_pair<pointer, deleter_type> __ptr_;
@@ -276,6 +289,17 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
typedef _Dp deleter_type;
typedef typename __pointer<_Tp, deleter_type>::type pointer;
+ // A unique_ptr contains the following members which may be trivially relocatable:
+ // - pointer : this may be trivially relocatable, so it's checked
+ // - deleter_type: this may be trivially relocatable, so it's checked
+ //
+ // This unique_ptr implementation only contains a pointer to the unique object and a deleter, so there are no
+ // references to itself. This means that the entire structure is trivially relocatable if its members are.
+ using __trivially_relocatable = __conditional_t<
+ __libcpp_is_trivially_relocatable<pointer>::value && __libcpp_is_trivially_relocatable<deleter_type>::value,
+ unique_ptr,
+ void>;
+
private:
__compressed_pair<pointer, deleter_type> __ptr_;
diff --git a/libcxx/include/__type_traits/is_trivially_relocatable.h b/libcxx/include/__type_traits/is_trivially_relocatable.h
new file mode 100644
index 0000000000000..c0871731cc001
--- /dev/null
+++ b/libcxx/include/__type_traits/is_trivially_relocatable.h
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___TYPE_TRAITS_IS_TRIVIALLY_RELOCATABLE_H
+#define _LIBCPP___TYPE_TRAITS_IS_TRIVIALLY_RELOCATABLE_H
+
+#include <__config>
+#include <__type_traits/enable_if.h>
+#include <__type_traits/integral_constant.h>
+#include <__type_traits/is_same.h>
+#include <__type_traits/is_trivially_copyable.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// A type is trivially relocatable if a move construct + destroy of the original object is equivalent to
+// `memcpy(dst, src, sizeof(T))`.
+
+#if __has_builtin(__is_trivially_relocatable)
+template <class _Tp, class = void>
+struct __libcpp_is_trivially_relocatable : integral_constant<bool, __is_trivially_relocatable(_Tp)> {};
+#else
+template <class _Tp, class = void>
+struct __libcpp_is_trivially_relocatable : is_trivially_copyable<_Tp> {};
+#endif
+
+template <class _Tp>
+struct __libcpp_is_trivially_relocatable<_Tp,
+ __enable_if_t<is_same<_Tp, typename _Tp::__trivially_relocatable>::value> >
+ : true_type {};
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___TYPE_TRAITS_IS_TRIVIALLY_RELOCATABLE_H
diff --git a/libcxx/include/libcxx.imp b/libcxx/include/libcxx.imp
index 13c0dfeb2bc98..8509c7e39a666 100644
--- a/libcxx/include/libcxx.imp
+++ b/libcxx/include/libcxx.imp
@@ -796,6 +796,7 @@
{ include: [ "<__type_traits/is_trivially_lexicographically_comparable.h>", "private", "<type_traits>", "public" ] },
{ include: [ "<__type_traits/is_trivially_move_assignable.h>", "private", "<type_traits>", "public" ] },
{ include: [ "<__type_traits/is_trivially_move_constructible.h>", "private", "<type_traits>", "public" ] },
+ { include: [ "<__type_traits/is_trivially_relocatable.h>", "private", "<type_traits>", "public" ] },
{ include: [ "<__type_traits/is_unbounded_array.h>", "private", "<type_traits>", "public" ] },
{ include: [ "<__type_traits/is_union.h>", "private", "<type_traits>", "public" ] },
{ include: [ "<__type_traits/is_unsigned.h>", "private", "<type_traits>", "public" ] },
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 03f71290b6603..ddb0f4b70e02b 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -1970,6 +1970,7 @@ module std_private_type_traits_is_trivially_destructible [system
module std_private_type_traits_is_trivially_lexicographically_comparable [system] { header "__type_traits/is_trivially_lexicographically_comparable.h" }
module std_private_type_traits_is_trivially_move_assignable [system] { header "__type_traits/is_trivially_move_assignable.h" }
module std_private_type_traits_is_trivially_move_constructible [system] { header "__type_traits/is_trivially_move_constructible.h" }
+module std_private_type_traits_is_trivially_relocatable [system] { header "__type_traits/is_trivially_relocatable.h" }
module std_private_type_traits_is_unbounded_array [system] { header "__type_traits/is_unbounded_array.h" }
module std_private_type_traits_is_union [system] { header "__type_traits/is_union.h" }
module std_private_type_traits_is_unsigned [system] { header "__type_traits/is_unsigned.h" }
diff --git a/libcxx/include/string b/libcxx/include/string
index efdff3dd42da0..ed4fdbe6864c2 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -599,6 +599,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
#include <__ranges/size.h>
#include <__string/char_traits.h>
#include <__string/extern_template_lists.h>
+#include <__type_traits/conditional.h>
#include <__type_traits/is_allocator.h>
#include <__type_traits/is_array.h>
#include <__type_traits/is_convertible.h>
@@ -607,6 +608,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
#include <__type_traits/is_same.h>
#include <__type_traits/is_standard_layout.h>
#include <__type_traits/is_trivial.h>
+#include <__type_traits/is_trivially_relocatable.h>
#include <__type_traits/noexcept_move_assign_container.h>
#include <__type_traits/remove_cvref.h>
#include <__type_traits/void_t.h>
@@ -724,6 +726,20 @@ public:
typedef typename __alloc_traits::pointer pointer;
typedef typename __alloc_traits::const_pointer const_pointer;
+ // A basic_string contains the following members which may be trivially relocatable:
+ // - pointer: is currently assumed to be trivially relocatable, but is still checked in case that changes
+ // - size_type: is always trivially relocatable, since it has to be an integral type
+ // - value_type: is always trivially relocatable, since it has to be trivial
+ // - unsigned char: is a fundamental type, so it's trivially relocatable
+ // - allocator_type: may or may not be trivially relocatable, so it's checked
+ //
+ // This string implementation doesn't contain any references into itself. It only contains a bit that says whether
+ // it is in small or large string mode, so the entire structure is trivially relocatable if its members are.
+ using __trivially_relocatable = __conditional_t<
+ __libcpp_is_trivially_relocatable<allocator_type>::value && __libcpp_is_trivially_relocatable<pointer>::value,
+ basic_string,
+ void>;
+
static_assert((!is_array<value_type>::value), "Character type of basic_string must not be an array");
static_assert((is_standard_layout<value_type>::value), "Character type of basic_string must be standard-layout");
static_assert((is_trivial<value_type>::value), "Character type of basic_string must be trivial");
diff --git a/libcxx/include/vector b/libcxx/include/vector
index 5e2027ea21a39..e9615ab4c9a30 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -982,14 +982,18 @@ template <ranges::input_range _Range,
vector(from_range_t, _Range&&, _Alloc = _Alloc()) -> vector<ranges::range_value_t<_Range>, _Alloc>;
#endif
+// __swap_out_circular_buffer relocates the objects in [__begin_, __end_) into the front of __v and swaps the buffers of
+// *this and __v. It is assumed that __v provides space for exactly (__end_ - __begin_) objects in the front. This
+// function has a strong exception guarantee.
template <class _Tp, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void
vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, allocator_type&>& __v) {
__annotate_delete();
- using _RevIter = std::reverse_iterator<pointer>;
- __v.__begin_ = std::__uninitialized_allocator_move_if_noexcept(
- __alloc(), _RevIter(__end_), _RevIter(__begin_), _RevIter(__v.__begin_))
- .base();
+ auto __new_begin = __v.__begin_ - (__end_ - __begin_);
+ std::__uninitialized_allocator_relocate(
+ __alloc(), std::__to_address(__begin_), std::__to_address(__end_), std::__to_address(__new_begin));
+ __v.__begin_ = __new_begin;
+ __end_ = __begin_; // All the objects have been destroyed by relocating them.
std::swap(this->__begin_, __v.__begin_);
std::swap(this->__end_, __v.__end_);
std::swap(this->__end_cap(), __v.__end_cap());
@@ -997,22 +1001,35 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, a
__annotate_new(size());
}
+// __swap_out_circular_buffer relocates the objects in [__begin_, __p) into the front of __v, the objects in
+// [__p, __end_) into the back of __v and swaps the buffers of *this and __v. It is assumed that __v provides space for
+// exactly (__p - __begin_) objects in the front and space for at least (__end_ - __p) objects in the back. This
+// function has a strong exception guarantee if __begin_ == __p || __end_ == __p.
template <class _Tp, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::pointer
vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, allocator_type&>& __v, pointer __p) {
__annotate_delete();
- pointer __r = __v.__begin_;
- using _RevIter = std::reverse_iterator<pointer>;
- __v.__begin_ = std::__uninitialized_allocator_move_if_noexcept(
- __alloc(), _RevIter(__p), _RevIter(__begin_), _RevIter(__v.__begin_))
- .base();
- __v.__end_ = std::__uninitialized_allocator_move_if_noexcept(__alloc(), __p, __end_, __v.__end_);
+ pointer __ret = __v.__begin_;
+
+ // Relocate [__p, __end_) first to avoid having a hole in [__begin_, __end_)
+ // in case something in [__begin_, __p) throws.
+ std::__uninitialized_allocator_relocate(
+ __alloc(), std::__to_address(__p), std::__to_address(__end_), std::__to_address(__v.__end_));
+ __v.__end_ += (__end_ - __p);
+ __end_ = __p; // The objects in [__p, __end_) have b...
[truncated]
|
I'll land this when the CI is happy. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
7c737cb
to
0e0ccde
Compare
…able types (llvm#76657)"" This reverts commit 2352fdd.
0e0ccde
to
e4c10a6
Compare
We're seeing compile errors after this, e.g.
Is this expected? |
@nico @gribozavr is correct that this is because of |
Agreed that they aren't allowed - but they have been (defacto, or intentionally - given the mention of the (might be worth reverting this patch to consider the fallout/whether there's a smoother way to stage this transition/removal?) |
(oh, the "I think I'd consider this a bug for now" - a bug in this patch? (yeah, for now, but it'd be good to remove the functionality/expectation eventually) - OK, makes sense, guess we're on the same page already - excuse the extra verbiage, then) |
…table types" (llvm#80558)" This reverts commit 4e112e5.
…pes" (llvm#80558) This reapplies llvm#76657. Non-trivial elements didn't get destroyed previously. This fixes the bug and adds tests for all the vector insertion functions.
#80558 introduced code that assumed that the element type of `vector` is never const. This fixes it and adds a test. Eventually we should remove the `allocator<const T>` extension.
The clang-tidy check We internally do use |
This reapplies #76657. Non-trivial elements didn't get destroyed previously. This fixes the bug and adds tests for all the vector insertion functions.