-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc++] Use relocation in vector::emplace_back #159365
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesFull diff: https://github.com/llvm/llvm-project/pull/159365.diff 2 Files Affected:
diff --git a/libcxx/include/__utility/exception_guard.h b/libcxx/include/__utility/exception_guard.h
index 6fa744e8b4f32..9c1a7b4b124d1 100644
--- a/libcxx/include/__utility/exception_guard.h
+++ b/libcxx/include/__utility/exception_guard.h
@@ -80,7 +80,10 @@ struct __exception_guard_exceptions {
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __complete() _NOEXCEPT { __completed_ = true; }
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 ~__exception_guard_exceptions() {
+ // __exception_guard is almost always used with a lamdba, so the destructor is almost certainly unique to the calling
+ // function. LLVM doesn't know that, so it doesn't inline it due to the destructor being in a cold (exception) path.
+ // Annotate the function with always_inline to work around those problems.
+ _LIBCPP_ALWAYS_INLINE _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 ~__exception_guard_exceptions() {
if (!__completed_)
__rollback_();
}
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 27e681aeef22a..483ef4f915453 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -1188,19 +1188,43 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
void
#endif
vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
- pointer __end = this->__end_;
- std::__if_likely_else(
- __end < this->__cap_,
- [&] {
- __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
- ++__end;
- },
- [&] { __end = __emplace_back_slow_path(std::forward<_Args>(__args)...); });
-
- this->__end_ = __end;
+ if constexpr (__libcpp_is_trivially_relocatable<value_type>::value &&
+ __allocator_has_trivial_move_construct_v<allocator_type, value_type> &&
+ __allocator_has_trivial_destroy_v<allocator_type, value_type>) {
+ union _Tmp {
+ _LIBCPP_CONSTEXPR_SINCE_CXX20 _Tmp() {}
+ _LIBCPP_CONSTEXPR_SINCE_CXX20 ~_Tmp() {}
+ value_type __val_;
+ };
+ _Tmp __tmp;
+
+ __alloc_traits::construct(__alloc_, std::addressof(__tmp.__val_), std::forward<_Args>(__args)...);
+
+ auto __guard =
+ std::__make_exception_guard([&, this] { __alloc_traits::destroy(__alloc_, std::addressof(__tmp.__val_)); });
+ std::__if_likely_else(size() != capacity(), [] {}, [this] { reserve(__recommend(size() + 1)); });
+ __guard.__complete();
+ std::__uninitialized_allocator_relocate(
+ __alloc_, std::addressof(__tmp.__val_), std::addressof(__tmp.__val_) + 1, std::__to_address(__end_));
+ ++__end_;
+#if _LIBCPP_STD_VER >= 17
+ return __end_[-1];
+#endif
+ } else {
+ pointer __end = this->__end_;
+ std::__if_likely_else(
+ __end < this->__cap_,
+ [&] {
+ __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
+ ++__end;
+ },
+ [&] { __end = __emplace_back_slow_path(std::forward<_Args>(__args)...); });
+
+ this->__end_ = __end;
#if _LIBCPP_STD_VER >= 17
- return *(__end - 1);
+ return *(__end - 1);
#endif
+ }
}
template <class _Tp, class _Allocator>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add benchmark data to your commit message?
libcxx/include/__vector/vector.h
Outdated
|
||
this->__end_ = __end; | ||
if constexpr (__libcpp_is_trivially_relocatable<value_type>::value && | ||
__allocator_has_trivial_move_construct_v<allocator_type, value_type> && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought: we might want to rename this to __allocator_has_transparent_move_construct_v
& friends.
libcxx/include/__vector/vector.h
Outdated
}; | ||
_Tmp __tmp; | ||
|
||
__alloc_traits::construct(__alloc_, std::addressof(__tmp.__val_), std::forward<_Args>(__args)...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a test that triggers this code path and the construction throws. Also we need a test with this code path where the reallocation throws, such that if we didn't have the __guard
then we'd fail the test.
99f785c
to
2355d80
Compare
No description provided.