[libc++] Add move constructor & assignment to exception_ptr
#164281
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vogelsgesang
force-pushed
the
avogelsgesang-exceptionptr-move
branch
from
fef6eee to
b595542
Compare
vogelsgesang
force-pushed
the
avogelsgesang-exceptionptr-move
branch
from
b595542 to
154c286
Compare
philnik777
left a comment
philnik777
left a comment
There was a problem hiding this comment.
Basically LGTM, just some nits.
|
I updated this commit to use implement the move-assignment using Is that what you had in mind? Benchmark results (original implementation) Benchmark results (swap-based implementation) Interpretation of benchmark results I consider the In particular The As soon as we inline the destructor, this regression should disappear again. As such, I think we can live with that temporary regression - WDYT? |
Yes, exactly.
Yeah, I think that's fine. Also, please update the commit message to mention that we introduce |
👍 then I think we have high-level alignment on this PR. The next step will be to polish it for final review.
I will do so after #164278 shipped, so I won't have to repeatedly rebase this PR and redo the measurements anymore One more question: |
Yeah, that should fix the CI. |
This commit adds move constructor and move assignment to
`exception_ptr`. Adding those operators allows us to avoid unnecessary
calls to `__cxa_{inc,dec}rement_refcount`.
Performance results:
```
Benchmark Baseline Candidate Difference % Difference
------------------------------- ---------- ----------- ------------ --------------
bm_nonnull_exception_ptr 52.22 40.92 -11.31 -21.65
bm_null_exception_ptr 31.41 23.29 -8.12 -25.85
bm_optimized_null_exception_ptr 28.69 20.50 -8.19 -28.55
```
This commit does not add a `swap` specialization. Thanks to the added
move-assignment, we already save a couple of increments/decrements also
in the default `swap` implementation. The default `swap` is still not
perfect, as it calls the desctructor on `tmp`. As soon as we also
inlined the `~exception_ptr` destructor fast-path for `__ptr ==
nullptr`, the optimizer should be able to optimize the default `swap`
just as well as a specialized `swap`, though.
vogelsgesang
force-pushed
the
avogelsgesang-exceptionptr-move
branch
from
154c286 to
0fcaeee
Compare
vogelsgesang
force-pushed
the
avogelsgesang-exceptionptr-move
branch
from
0fcaeee to
b106dc9
Compare
|
/libcxx-bot benchmark libcxx/test/benchmarks/exception_ptr.bench.cpp Benchmark results: |
philnik777
left a comment
philnik777
left a comment
There was a problem hiding this comment.
Could you add some basic tests for the new functions? I'm pretty sure everything should be testable portably to some extent.
Co-authored-by: Nikolas Klauser <[email protected]>
philnik777
left a comment
philnik777
left a comment
There was a problem hiding this comment.
Except for also improving the MSVC version this looks basically good to me.
...xx/test/std/language.support/support.exception/propagation/exception_ptr_move_assignment.cpp
Outdated
Show resolved
Hide resolved
...st/std/language.support/support.exception/propagation/exception_ptr_move_assignment.pass.cpp
Show resolved
Hide resolved
...xx/test/std/language.support/support.exception/propagation/exception_ptr_move_assignment.cpp
Outdated
Show resolved
Hide resolved
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Adrian Vogelsgesang (vogelsgesang) ChangesThis commit adds move constructor and move assignment and Performance results: As expected, the While Full diff: https://github.com/llvm/llvm-project/pull/164281.diff 5 Files Affected:
diff --git a/libcxx/include/__exception/exception_ptr.h b/libcxx/include/__exception/exception_ptr.h
index 796fa924be121..e096d8e1dfd49 100644
--- a/libcxx/include/__exception/exception_ptr.h
+++ b/libcxx/include/__exception/exception_ptr.h
@@ -16,6 +16,7 @@
#include <__memory/construct_at.h>
#include <__type_traits/decay.h>
#include <__type_traits/is_pointer.h>
+#include <__utility/move.h>
#include <cstdlib>
#include <typeinfo>
@@ -23,6 +24,9 @@
# pragma GCC system_header
#endif
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
#ifndef _LIBCPP_ABI_MICROSOFT
# if _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION
@@ -75,7 +79,15 @@ class _LIBCPP_EXPORTED_FROM_ABI exception_ptr {
_LIBCPP_HIDE_FROM_ABI exception_ptr(nullptr_t) _NOEXCEPT : __ptr_() {}
exception_ptr(const exception_ptr&) _NOEXCEPT;
+ _LIBCPP_HIDE_FROM_ABI exception_ptr(exception_ptr&& __other) _NOEXCEPT : __ptr_(__other.__ptr_) {
+ __other.__ptr_ = nullptr;
+ }
exception_ptr& operator=(const exception_ptr&) _NOEXCEPT;
+ _LIBCPP_HIDE_FROM_ABI exception_ptr& operator=(exception_ptr&& __other) _NOEXCEPT {
+ exception_ptr __tmp(std::move(__other));
+ std::swap(__tmp, *this);
+ return *this;
+ }
~exception_ptr() _NOEXCEPT;
_LIBCPP_HIDE_FROM_ABI explicit operator bool() const _NOEXCEPT { return __ptr_ != nullptr; }
@@ -88,10 +100,16 @@ class _LIBCPP_EXPORTED_FROM_ABI exception_ptr {
return !(__x == __y);
}
+ friend _LIBCPP_HIDE_FROM_ABI void swap(exception_ptr& __x, exception_ptr& __y) _NOEXCEPT;
+
friend _LIBCPP_EXPORTED_FROM_ABI exception_ptr current_exception() _NOEXCEPT;
friend _LIBCPP_EXPORTED_FROM_ABI void rethrow_exception(exception_ptr);
};
+inline _LIBCPP_HIDE_FROM_ABI void swap(exception_ptr& __x, exception_ptr& __y) _NOEXCEPT {
+ swap(__x.__ptr_, __y.__ptr_);
+}
+
# if _LIBCPP_HAS_EXCEPTIONS
# if _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION
template <class _Ep>
@@ -201,4 +219,6 @@ _LIBCPP_HIDE_FROM_ABI exception_ptr make_exception_ptr(_Ep __e) _NOEXCEPT {
#endif // _LIBCPP_ABI_MICROSOFT
_LIBCPP_END_UNVERSIONED_NAMESPACE_STD
+_LIBCPP_POP_MACROS
+
#endif // _LIBCPP___EXCEPTION_EXCEPTION_PTR_H
diff --git a/libcxx/test/std/language.support/support.exception/propagation/exception_ptr.pass.cpp b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr.pass.cpp
index 0aded33e660d5..7e25d40dc8a7d 100644
--- a/libcxx/test/std/language.support/support.exception/propagation/exception_ptr.pass.cpp
+++ b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr.pass.cpp
@@ -14,7 +14,6 @@
#include <exception>
#include <cassert>
-#include <type_traits>
#include "test_macros.h"
diff --git a/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_move_assignment.cpp b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_move_assignment.cpp
new file mode 100644
index 0000000000000..3f041fc367dfd
--- /dev/null
+++ b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_move_assignment.cpp
@@ -0,0 +1,45 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-exceptions
+
+// <exception>
+
+// typedef unspecified exception_ptr;
+
+// Test the move assignment of exception_ptr
+
+#include <exception>
+#include <utility>
+#include <cassert>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+ std::exception_ptr p = std::make_exception_ptr(42);
+ std::exception_ptr p2{p};
+ assert(p2 == p);
+ // Under test: the move assignment
+ std::exception_ptr p3;
+ p3 = std::move(p2);
+ assert(p3 == p);
+// `p2` was moved from. In libc++ it will be nullptr, but
+// this is not guaranteed by the standard.
+#if defined(_LIBCPP_VERSION) && !defined(_LIBCPP_ABI_MICROSOFT)
+ assert(p2 == nullptr);
+ assert(p2 == nullptr);
+#endif
+
+ try {
+ std::rethrow_exception(p3);
+ } catch (int e) {
+ assert(e == 42);
+ }
+
+ return 0;
+}
\ No newline at end of file
diff --git a/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_move_ctr.cpp b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_move_ctr.cpp
new file mode 100644
index 0000000000000..37421e86a3d3f
--- /dev/null
+++ b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_move_ctr.cpp
@@ -0,0 +1,43 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-exceptions
+
+// <exception>
+
+// typedef unspecified exception_ptr;
+
+// Test the move constructor of exception_ptr
+
+#include <exception>
+#include <utility>
+#include <cassert>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+ std::exception_ptr p = std::make_exception_ptr(42);
+ std::exception_ptr p2{p};
+ assert(p2 == p);
+ // Under test: The move constructor
+ std::exception_ptr p3{std::move(p2)};
+ assert(p3 == p);
+// `p2` was moved from. In libc++ it will be nullptr, but
+// this is not guaranteed by the standard.
+#if defined(_LIBCPP_VERSION) && !defined(_LIBCPP_ABI_MICROSOFT)
+ assert(p2 == nullptr);
+#endif
+
+ try {
+ std::rethrow_exception(p3);
+ } catch (int e) {
+ assert(e == 42);
+ }
+
+ return 0;
+}
\ No newline at end of file
diff --git a/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_swap.cpp b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_swap.cpp
new file mode 100644
index 0000000000000..82b4713bed538
--- /dev/null
+++ b/libcxx/test/std/language.support/support.exception/propagation/exception_ptr_swap.cpp
@@ -0,0 +1,40 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-exceptions
+
+// <exception>
+
+// typedef unspecified exception_ptr;
+
+// Test swapping of exception_ptr
+
+#include <exception>
+#include <utility>
+#include <cassert>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+ std::exception_ptr p21 = std::make_exception_ptr(42);
+ std::exception_ptr p42 = std::make_exception_ptr(21);
+ std::swap(p42, p21);
+
+ try {
+ std::rethrow_exception(p21);
+ } catch (int e) {
+ assert(e == 21);
+ }
+ try {
+ std::rethrow_exception(p42);
+ } catch (int e) {
+ assert(e == 42);
+ }
+
+ return 0;
+}
|
vogelsgesang
force-pushed
the
avogelsgesang-exceptionptr-move
branch
from
08b0a14 to
ee4694c
Compare
philnik777
left a comment
philnik777
left a comment
There was a problem hiding this comment.
LGTM % nit. I think you need to update the numbers in the commit message though.
...xx/test/std/language.support/support.exception/propagation/exception_ptr_move_assignment.cpp
Outdated
Show resolved
Hide resolved
I am planning to use the PR description as my commit message. Afaict, the performance numbers in in the PR description should be up-to-date by now. Note that I used the "official CI" numbers (measured by your CI and posted to this comment) instead of my local measurements. Or did I overlook something? |
Ah, sorry, I thought you'd go for your numbers. The CI isn't super well tested, so I tend to prefer local numbers. But these look close enough to yours that it's probably fine. Never mind then. |
vogelsgesang
left a comment
vogelsgesang
left a comment
There was a problem hiding this comment.
@philnik777 Thanks for your approval. I had to do a couple more changes to (hopefully) get the CI green.
In particular on the two marked changes I am not 100% sure
| using std::rethrow_exception; | ||
| using std::rethrow_if_nested; | ||
| using std::set_terminate; | ||
| using std::swap; |
There was a problem hiding this comment.
I had to add std::swap to this list since otherwise
the test cases libcxx/module_std.gen.py/module_std.sh.cpp and libcxx/module_std_compat.gen.py/module_std_compat.sh.cpp failed.
I don't quite understand the modules build, though. Is this correct?
(Note that we now have both a std::swap and a std::_LIBCPP_ABI_NAMESPACE::swap because exception_ptr is in the unversioned namespace while the normal swap is in the versioned namespace. Not sure if that matters. I hope not 🤞 )
There was a problem hiding this comment.
I don't quite understand how these tests work either. I'm happy if the CI is happy here.
There was a problem hiding this comment.
The CI was indeed green 🙂
I merged this (after removing the documentation update from this PR)
Moving on to #165909
3 participants
This commit adds move constructor and move assignment and
swapto
exception_ptr. Adding those operators allows us to avoidunnecessary calls to
__cxa_{inc,dec}rement_refcount.Performance results (from libc++'s CI):
As expected, the
bm_exception_ptr_copy_*benchmarks are not influenced by this change.The
bm_exception_ptr_move_*benefit between 18% and 30%.The
bm_exception_ptr_swap_*tests show the biggest improvements since multiple calls to the copy constructor are replaced by a simple pointer swap.While
bm_exception_ptr_move_assign_nulldid not show a regression in the CI measurements, local measurements showed a regression from 3.98 to 4.71, i.e. by 18%. This is due to the additional__tmpinsideoperator=. The destructor of the original__otheris a no-op after the move because__other.__ptrwill be a nullptr. However, the compiler does not realize this, since the destructor is not inlined and is lacking a fast-path. As such, the swap-based implementation leads to an additional destructor call. Thebm_exception_ptr_move_assign_nonnullstill benefits because the swap-based move constructor avoids unnecessary _cxa{in,de}crement_refcount calls. As soon as we inline the destructor, this regression should disappear again.Works towards #44892