-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[libc++] Inline fast path forexception_ptr copy constructor & destructor
#165909
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?
[libc++] Inline fast path forexception_ptr copy constructor & destructor
#165909
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
38269ec to
d332adc
Compare
|
/libcxx-bot benchmark libcxx/test/benchmarks/exception_ptr.bench.cpp Benchmark results: |
|
@vogelsgesang Do you think this is ready for review? If so, please make it ready for review. |
|
@llvm/pr-subscribers-libcxx Author: Adrian Vogelsgesang (vogelsgesang) ChangesNot ready for detailed review, yet. But two high-level questions around ABI stability could already be discussed:
This commit adds an inlined fast-path for nullptrs to the copy constructor, copy assignment and destructor of Performance results (from libc++'s CI): Full diff: https://github.com/llvm/llvm-project/pull/165909.diff 5 Files Affected:
diff --git a/libcxx/include/__exception/exception_ptr.h b/libcxx/include/__exception/exception_ptr.h
index e78126ea23852..0f081c6bc8ed0 100644
--- a/libcxx/include/__exception/exception_ptr.h
+++ b/libcxx/include/__exception/exception_ptr.h
@@ -30,6 +30,28 @@ _LIBCPP_PUSH_MACROS
#ifndef _LIBCPP_ABI_MICROSOFT
+// Previously, parts of exception_ptr were defined out-of-line, which prevented
+// useful compiler optimizations. Changing the out-of-line definitions to inline
+// definitions is an ABI break, however. To prevent this, we have to make sure
+// the symbols remain available in the libc++ library, in addition to being
+// defined inline here in this header.
+// To this end, we use _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE macro:
+// The macro is defined as empty for src/exception.cpp, forcing the definitions of
+// the functions to be emitted and included in the library. When users of libc++
+// compile their code, the __gnu_inline__ attribute will suppress generation of
+// these functions while making their definitions available for inlining.
+# ifdef _LIBCPP_EMIT_CODE_FOR_EXCEPTION_PTR
+# define _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE _LIBCPP_EXPORTED_FROM_ABI
+# else
+# if !__has_cpp_attribute(__gnu__::__gnu_inline__)
+# error "GNU inline attribute is not supported"
+# endif
+# define _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE [[__gnu__::__gnu_inline__]] inline
+# endif
+
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wgnu-inline-cpp-without-extern")
+
# if _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION
namespace __cxxabiv1 {
@@ -67,6 +89,17 @@ inline _LIBCPP_HIDE_FROM_ABI void swap(exception_ptr& __x, exception_ptr& __y) _
class _LIBCPP_EXPORTED_FROM_ABI exception_ptr {
void* __ptr_;
+ static void __do_increment_refcount(void* __ptr) _NOEXCEPT;
+ static void __do_decrement_refcount(void* __ptr) _NOEXCEPT;
+ _LIBCPP_HIDE_FROM_ABI static void __increment_refcount(void* __ptr) _NOEXCEPT {
+ if (__ptr)
+ __do_increment_refcount(__ptr);
+ }
+ _LIBCPP_HIDE_FROM_ABI static void __decrement_refcount(void* __ptr) _NOEXCEPT {
+ if (__ptr)
+ __do_decrement_refcount(__ptr);
+ }
+
static exception_ptr __from_native_exception_pointer(void*) _NOEXCEPT;
template <class _Ep>
@@ -81,17 +114,18 @@ class _LIBCPP_EXPORTED_FROM_ABI exception_ptr {
_LIBCPP_HIDE_FROM_ABI exception_ptr() _NOEXCEPT : __ptr_() {}
_LIBCPP_HIDE_FROM_ABI exception_ptr(nullptr_t) _NOEXCEPT : __ptr_() {}
- exception_ptr(const exception_ptr&) _NOEXCEPT;
+ _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE 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_EXPORTED_FROM_LIB_INLINEABLE 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);
+ __decrement_refcount(__ptr_);
+ __ptr_ = __other.__ptr_;
+ __other.__ptr_ = nullptr;
return *this;
}
- ~exception_ptr() _NOEXCEPT;
+ _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE ~exception_ptr() _NOEXCEPT;
_LIBCPP_HIDE_FROM_ABI explicit operator bool() const _NOEXCEPT { return __ptr_ != nullptr; }
@@ -109,6 +143,25 @@ class _LIBCPP_EXPORTED_FROM_ABI exception_ptr {
friend _LIBCPP_EXPORTED_FROM_ABI void rethrow_exception(exception_ptr);
};
+// Must be defined outside the class definition due to _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE
+_LIBCPP_EXPORTED_FROM_LIB_INLINEABLE exception_ptr::exception_ptr(const exception_ptr& __other) _NOEXCEPT
+ : __ptr_(__other.__ptr_) {
+ __increment_refcount(__ptr_);
+}
+
+// Must be defined outside the class definition due to _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE
+_LIBCPP_EXPORTED_FROM_LIB_INLINEABLE exception_ptr& exception_ptr::operator=(const exception_ptr& __other) _NOEXCEPT {
+ if (__ptr_ != __other.__ptr_) {
+ __increment_refcount(__other.__ptr_);
+ __decrement_refcount(__ptr_);
+ __ptr_ = __other.__ptr_;
+ }
+ return *this;
+}
+
+// Must be defined outside the class definition due to _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE
+_LIBCPP_EXPORTED_FROM_LIB_INLINEABLE exception_ptr::~exception_ptr() _NOEXCEPT { __decrement_refcount(__ptr_); }
+
inline _LIBCPP_HIDE_FROM_ABI void swap(exception_ptr& __x, exception_ptr& __y) _NOEXCEPT {
std::swap(__x.__ptr_, __y.__ptr_);
}
@@ -222,6 +275,8 @@ _LIBCPP_HIDE_FROM_ABI exception_ptr make_exception_ptr(_Ep __e) _NOEXCEPT {
#endif // _LIBCPP_ABI_MICROSOFT
_LIBCPP_END_UNVERSIONED_NAMESPACE_STD
+_LIBCPP_DIAGNOSTIC_POP
+
_LIBCPP_POP_MACROS
#endif // _LIBCPP___EXCEPTION_EXCEPTION_PTR_H
diff --git a/libcxx/src/exception.cpp b/libcxx/src/exception.cpp
index ac6324cd9fe35..9dd9b0c9938fd 100644
--- a/libcxx/src/exception.cpp
+++ b/libcxx/src/exception.cpp
@@ -8,6 +8,7 @@
#define _LIBCPP_ENABLE_CXX20_REMOVED_UNCAUGHT_EXCEPTION
#define _LIBCPP_DISABLE_DEPRECATION_WARNINGS
+#define _LIBCPP_EMIT_CODE_FOR_EXCEPTION_PTR
#include <exception>
#include <new>
diff --git a/libcxx/src/support/runtime/exception_pointer_cxxabi.ipp b/libcxx/src/support/runtime/exception_pointer_cxxabi.ipp
index 8f5c2060bb06c..e09bf8981263f 100644
--- a/libcxx/src/support/runtime/exception_pointer_cxxabi.ipp
+++ b/libcxx/src/support/runtime/exception_pointer_cxxabi.ipp
@@ -13,19 +13,12 @@
namespace std {
-exception_ptr::~exception_ptr() noexcept { __cxa_decrement_exception_refcount(__ptr_); }
-
-exception_ptr::exception_ptr(const exception_ptr& other) noexcept : __ptr_(other.__ptr_) {
- __cxa_increment_exception_refcount(__ptr_);
+void exception_ptr::__do_increment_refcount(void* __ptr) noexcept {
+ __cxa_increment_exception_refcount(__ptr);
}
-exception_ptr& exception_ptr::operator=(const exception_ptr& other) noexcept {
- if (__ptr_ != other.__ptr_) {
- __cxa_increment_exception_refcount(other.__ptr_);
- __cxa_decrement_exception_refcount(__ptr_);
- __ptr_ = other.__ptr_;
- }
- return *this;
+void exception_ptr::__do_decrement_refcount(void* __ptr) noexcept {
+ __cxa_decrement_exception_refcount(__ptr);
}
exception_ptr exception_ptr::__from_native_exception_pointer(void* __e) noexcept {
diff --git a/libcxx/src/support/runtime/exception_pointer_glibcxx.ipp b/libcxx/src/support/runtime/exception_pointer_glibcxx.ipp
index 174b44ce0e6f7..c7b2e343b5f09 100644
--- a/libcxx/src/support/runtime/exception_pointer_glibcxx.ipp
+++ b/libcxx/src/support/runtime/exception_pointer_glibcxx.ipp
@@ -7,14 +7,14 @@
//
//===----------------------------------------------------------------------===//
+
// libsupc++ does not implement the dependent EH ABI and the functionality
// it uses to implement std::exception_ptr (which it declares as an alias of
// std::__exception_ptr::exception_ptr) is not directly exported to clients. So
// we have little choice but to hijack std::__exception_ptr::exception_ptr's
-// (which fortunately has the same layout as our std::exception_ptr) copy
-// constructor, assignment operator and destructor (which are part of its
-// stable ABI), and its rethrow_exception(std::__exception_ptr::exception_ptr)
-// function.
+// _M_addref and _M_release and its rethrow_exception function. Fortunately,
+// glibcxx's exception_ptr has the same layout as our exception_ptr and we can
+// reinterpret_cast between the two.
namespace std {
@@ -23,27 +23,20 @@ namespace __exception_ptr {
struct exception_ptr {
void* __ptr_;
- explicit exception_ptr(void*) noexcept;
- exception_ptr(const exception_ptr&) noexcept;
- exception_ptr& operator=(const exception_ptr&) noexcept;
- ~exception_ptr() noexcept;
+ void _M_addref() noexcept;
+ void _M_release() noexcept;
};
} // namespace __exception_ptr
[[noreturn]] void rethrow_exception(__exception_ptr::exception_ptr);
-exception_ptr::~exception_ptr() noexcept { reinterpret_cast<__exception_ptr::exception_ptr*>(this)->~exception_ptr(); }
-
-exception_ptr::exception_ptr(const exception_ptr& other) noexcept : __ptr_(other.__ptr_) {
- new (reinterpret_cast<void*>(this))
- __exception_ptr::exception_ptr(reinterpret_cast<const __exception_ptr::exception_ptr&>(other));
+void exception_ptr::__do_increment_refcount(void* __ptr) noexcept {
+ reinterpret_cast<__exception_ptr::exception_ptr*>(this)->_M_addref();
}
-exception_ptr& exception_ptr::operator=(const exception_ptr& other) noexcept {
- *reinterpret_cast<__exception_ptr::exception_ptr*>(this) =
- reinterpret_cast<const __exception_ptr::exception_ptr&>(other);
- return *this;
+void exception_ptr::__do_decrement_refcount(void* __ptr) noexcept {
+ reinterpret_cast<__exception_ptr::exception_ptr*>(this)->_M_release();
}
exception_ptr exception_ptr::__from_native_exception_pointer(void* __e) noexcept {
diff --git a/libcxx/src/support/runtime/exception_pointer_unimplemented.ipp b/libcxx/src/support/runtime/exception_pointer_unimplemented.ipp
index 05a71ce34e5ac..78be16bf95188 100644
--- a/libcxx/src/support/runtime/exception_pointer_unimplemented.ipp
+++ b/libcxx/src/support/runtime/exception_pointer_unimplemented.ipp
@@ -11,17 +11,12 @@
namespace std {
-exception_ptr::~exception_ptr() noexcept {
+void exception_ptr::__do_increment_refcount(void* __ptr) noexcept {
#warning exception_ptr not yet implemented
__libcpp_verbose_abort("exception_ptr not yet implemented\n");
}
-exception_ptr::exception_ptr(const exception_ptr& other) noexcept : __ptr_(other.__ptr_) {
-#warning exception_ptr not yet implemented
- __libcpp_verbose_abort("exception_ptr not yet implemented\n");
-}
-
-exception_ptr& exception_ptr::operator=(const exception_ptr& other) noexcept {
+void exception_ptr::__do_decrement_refcount(void* __ptr) noexcept {
#warning exception_ptr not yet implemented
__libcpp_verbose_abort("exception_ptr not yet implemented\n");
}
|
The PR is ready for a first round of feedback, in particular on the high-level questions:
|
e06f411 to
7684774
Compare
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.
Also, please don't tag anybody in the commit message. That will cause notifications any time the commit is pushed anywhere on github otherwise.
| // Previously, parts of exception_ptr were defined out-of-line, which prevented | ||
| // useful compiler optimizations. Changing the out-of-line definitions to inline | ||
| // definitions is an ABI break, however. To prevent this, we have to make sure | ||
| // the symbols remain available in the libc++ library, in addition to being | ||
| // defined inline here in this header. | ||
| // To this end, we use _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE macro: | ||
| // The macro is defined as empty for src/exception.cpp, forcing the definitions of | ||
| // the functions to be emitted and included in the library. When users of libc++ | ||
| // compile their code, the __gnu_inline__ attribute will suppress generation of | ||
| // these functions while making their definitions available for inlining. | ||
| # ifdef _LIBCPP_EMIT_CODE_FOR_EXCEPTION_PTR | ||
| # define _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE _LIBCPP_EXPORTED_FROM_ABI | ||
| # else | ||
| # if !__has_cpp_attribute(__gnu__::__gnu_inline__) | ||
| # error "GNU inline attribute is not supported" | ||
| # endif | ||
| # define _LIBCPP_EXPORTED_FROM_LIB_INLINEABLE [[__gnu__::__gnu_inline__]] inline | ||
| # endif |
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.
I think we can drop this entire thing and instead just have an #ifdef _LIBCPP_BUILDING_LIBRARY, in which case we provide the old declarations, and otherwise provide inline functions. The inline functions get different mangling anyways, so I don't think there is much of a problem. I have actually no idea why I suggested this convoluted workaround before.
| _LIBCPP_HIDE_FROM_ABI static void __increment_refcount(void* __ptr) _NOEXCEPT { | ||
| if (__ptr) | ||
| __do_increment_refcount(__ptr); | ||
| } | ||
| _LIBCPP_HIDE_FROM_ABI static void __decrement_refcount(void* __ptr) _NOEXCEPT { | ||
| if (__ptr) | ||
| __do_decrement_refcount(__ptr); | ||
| } |
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.
I feel like these names are rather poor. It'd either inline these relatively simple functions or rename them so it's obvious they might inc/decrement the refcount.
| static void __do_increment_refcount(void* __ptr) _NOEXCEPT; | ||
| static void __do_decrement_refcount(void* __ptr) _NOEXCEPT; |
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.
| static void __do_increment_refcount(void* __ptr) _NOEXCEPT; | |
| static void __do_decrement_refcount(void* __ptr) _NOEXCEPT; | |
| static void __do_increment_refcount([[__gnu__::__nonnull__]] _LIBCPP_NOESCAPE void*) _NOEXCEPT; | |
| static void __do_decrement_refcount([[__gnu__::__nonnull__]] _LIBCPP_NOESCAPE void*) _NOEXCEPT; |
Not ready for detailed review, yet. But two high-level questions around ABI stability could already be discussed:
~exception_ptrand copy constructor/assignment in the library. The current approach was previously discussed in https://reviews.llvm.org/D122536 (among others by @philnik777).__do_{inc,dec}rementfunctions or if we want to directly call__cxa_{inc,dec}rement_refcountfrom the headersBut if we prefer a different approach, I am happy to adjust this PR accordingly
This commit adds an inlined fast-path for nullptrs to the copy constructor, copy assignment and destructor of
std::exception_ptr.Performance results (from libc++'s CI):