Reapply "[libc++] Optimize vector growing of trivially relocatable types" #80558
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
llvmbot added the libc++ 
| @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. |
philnik777 force-pushed the uninitilaized_allocator_relocate branch 3 times, most recently from 7c737cb to 0e0ccde Compare …able types (llvm#76657)"" This reverts commit 2352fdd.
philnik777 force-pushed the uninitilaized_allocator_relocate branch from 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) |
agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
…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.
pranavk pushed a commit that referenced this pull request Feb 5, 2024
#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 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This reapplies #76657. Non-trivial elements didn't get destroyed previously. This fixes the bug and adds tests for all the vector insertion functions.