<div dir="ltr">Do you know if this optimization works on Windows too?</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Jul 6, 2019 at 9:21 PM Eric Fiselier via libcxx-commits <<a href="mailto:libcxx-commits@lists.llvm.org">libcxx-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: ericwf<br>
Date: Sat Jul 6 18:20:54 2019<br>
New Revision: 365273<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=365273&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=365273&view=rev</a><br>
Log:<br>
Fix PR27658 - Make ~mutex trivial when possible.<br>
<br>
Currently std::mutex has a constexpr constructor, but a non-trivial<br>
destruction.<br>
<br>
The constexpr constructor is required to ensure the construction of a<br>
mutex with static storage duration happens at compile time, during<br>
constant initialization, and not during dynamic initialization.<br>
This means that static mutex's are always initialized and can be used<br>
safely during dynamic initialization without the "static initialization<br>
order fiasco".<br>
<br>
A trivial destructor is important for similar reasons. If a mutex is<br>
used during dynamic initialization it might also be used during program<br>
termination. If a static mutex has a non-trivial destructor it will be<br>
invoked during termination. This can introduce the "static<br>
deinitialization order fiasco".<br>
<br>
Additionally, function-local statics emit a guard variable around<br>
non-trivially destructible types. This results in horrible codegen and<br>
adds a runtime cost to every call to that function. non-local static's<br>
also result in slightly worse codegen but it's not as big of a problem.<br>
<br>
Example codegen can be found here: <a href="https://goo.gl/3CSzbM" rel="noreferrer" target="_blank">https://goo.gl/3CSzbM</a><br>
<br>
Note: This optimization is not safe with every pthread implementation.<br>
Some implementations allocate on the first call to pthread_mutex_lock<br>
and free the allocation in pthread_mutex_destroy.<br>
<br>
Also, changing the triviality of the destructor is not an ABI break.<br>
At least to the best of my knowledge :-)<br>
<br>
Added:<br>
libcxx/trunk/src/mutex_destructor.cpp<br>
Modified:<br>
libcxx/trunk/include/__config<br>
libcxx/trunk/include/__mutex_base<br>
libcxx/trunk/src/CMakeLists.txt<br>
libcxx/trunk/src/mutex.cpp<br>
libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp<br>
<br>
Modified: libcxx/trunk/include/__config<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config?rev=365273&r1=365272&r2=365273&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config?rev=365273&r1=365272&r2=365273&view=diff</a><br>
==============================================================================<br>
--- libcxx/trunk/include/__config (original)<br>
+++ libcxx/trunk/include/__config Sat Jul 6 18:20:54 2019<br>
@@ -1097,6 +1097,14 @@ _LIBCPP_FUNC_VIS extern "C" void __sanit<br>
_LIBCPP_HAS_NO_THREADS is defined.<br>
#endif<br>
<br>
+// The Apple, glibc, and Bionic implementation of pthreads implements<br>
+// pthread_mutex_destroy as nop for regular mutexes.<br>
+// TODO(EricWF): Enable this optimization on Apple and Bionic platforms after<br>
+// speaking to their respective stakeholders.<br>
+#if defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && defined(__GLIBC__)<br>
+# define _LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION<br>
+#endif<br>
+<br>
// Systems that use capability-based security (FreeBSD with Capsicum,<br>
// Nuxi CloudABI) may only provide local filesystem access (using *at()).<br>
// Functions like open(), rename(), unlink() and stat() should not be<br>
<br>
Modified: libcxx/trunk/include/__mutex_base<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__mutex_base?rev=365273&r1=365272&r2=365273&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__mutex_base?rev=365273&r1=365272&r2=365273&view=diff</a><br>
==============================================================================<br>
--- libcxx/trunk/include/__mutex_base (original)<br>
+++ libcxx/trunk/include/__mutex_base Sat Jul 6 18:20:54 2019<br>
@@ -36,28 +36,24 @@ _LIBCPP_BEGIN_NAMESPACE_STD<br>
# endif<br>
#endif // _LIBCPP_THREAD_SAFETY_ANNOTATION<br>
<br>
+<br>
class _LIBCPP_TYPE_VIS _LIBCPP_THREAD_SAFETY_ANNOTATION(capability("mutex")) mutex<br>
{<br>
-#ifndef _LIBCPP_CXX03_LANG<br>
__libcpp_mutex_t __m_ = _LIBCPP_MUTEX_INITIALIZER;<br>
-#else<br>
- __libcpp_mutex_t __m_;<br>
-#endif<br>
<br>
public:<br>
_LIBCPP_INLINE_VISIBILITY<br>
-#ifndef _LIBCPP_CXX03_LANG<br>
- constexpr mutex() = default;<br>
+ _LIBCPP_CONSTEXPR mutex() = default;<br>
+<br>
+ mutex(const mutex&) = delete;<br>
+ mutex& operator=(const mutex&) = delete;<br>
+<br>
+#if defined(_LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION)<br>
+ ~mutex() = default;<br>
#else<br>
- mutex() _NOEXCEPT {__m_ = (__libcpp_mutex_t)_LIBCPP_MUTEX_INITIALIZER;}<br>
-#endif<br>
~mutex() _NOEXCEPT;<br>
+#endif<br>
<br>
-private:<br>
- mutex(const mutex&);// = delete;<br>
- mutex& operator=(const mutex&);// = delete;<br>
-<br>
-public:<br>
void lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_capability());<br>
bool try_lock() _NOEXCEPT _LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_capability(true));<br>
void unlock() _NOEXCEPT _LIBCPP_THREAD_SAFETY_ANNOTATION(release_capability());<br>
<br>
Modified: libcxx/trunk/src/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/CMakeLists.txt?rev=365273&r1=365272&r2=365273&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/CMakeLists.txt?rev=365273&r1=365272&r2=365273&view=diff</a><br>
==============================================================================<br>
--- libcxx/trunk/src/CMakeLists.txt (original)<br>
+++ libcxx/trunk/src/CMakeLists.txt Sat Jul 6 18:20:54 2019<br>
@@ -22,6 +22,7 @@ set(LIBCXX_SOURCES<br>
locale.cpp<br>
memory.cpp<br>
mutex.cpp<br>
+ mutex_destructor.cpp<br>
new.cpp<br>
optional.cpp<br>
random.cpp<br>
<br>
Modified: libcxx/trunk/src/mutex.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/mutex.cpp?rev=365273&r1=365272&r2=365273&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/mutex.cpp?rev=365273&r1=365272&r2=365273&view=diff</a><br>
==============================================================================<br>
--- libcxx/trunk/src/mutex.cpp (original)<br>
+++ libcxx/trunk/src/mutex.cpp Sat Jul 6 18:20:54 2019<br>
@@ -25,10 +25,7 @@ const defer_lock_t defer_lock = {};<br>
const try_to_lock_t try_to_lock = {};<br>
const adopt_lock_t adopt_lock = {};<br>
<br>
-mutex::~mutex() _NOEXCEPT<br>
-{<br>
- __libcpp_mutex_destroy(&__m_);<br>
-}<br>
+// ~mutex is defined elsewhere<br>
<br>
void<br>
mutex::lock()<br>
<br>
Added: libcxx/trunk/src/mutex_destructor.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/mutex_destructor.cpp?rev=365273&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/mutex_destructor.cpp?rev=365273&view=auto</a><br>
==============================================================================<br>
--- libcxx/trunk/src/mutex_destructor.cpp (added)<br>
+++ libcxx/trunk/src/mutex_destructor.cpp Sat Jul 6 18:20:54 2019<br>
@@ -0,0 +1,51 @@<br>
+//===--------------------- mutex_destructor.cpp ---------------------------===//<br>
+//<br>
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.<br>
+// See <a href="https://llvm.org/LICENSE.txt" rel="noreferrer" target="_blank">https://llvm.org/LICENSE.txt</a> for license information.<br>
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+// Define ~mutex.<br>
+//<br>
+// On some platforms ~mutex has been made trivial and the definition is only<br>
+// provided for ABI compatibility.<br>
+//<br>
+// In order to avoid ODR violations within libc++ itself, we need to ensure<br>
+// that *nothing* sees the non-trivial mutex declaration. For this reason<br>
+// we re-declare the entire class in this file instead of using<br>
+// _LIBCPP_BUILDING_LIBRARY to change the definition in the headers.<br>
+<br>
+#include "__config"<br>
+#include "__threading_support"<br>
+<br>
+#if !defined(_LIBCPP_HAS_NO_THREADS)<br>
+#if _LIBCPP_ABI_VERSION == 1 || !defined(_LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION)<br>
+#define NEEDS_MUTEX_DESTRUCTOR<br>
+#endif<br>
+#endif<br>
+<br>
+_LIBCPP_BEGIN_NAMESPACE_STD<br>
+<br>
+#ifdef NEEDS_MUTEX_DESTRUCTOR<br>
+class _LIBCPP_TYPE_VIS mutex<br>
+{<br>
+ __libcpp_mutex_t __m_ = _LIBCPP_MUTEX_INITIALIZER;<br>
+<br>
+public:<br>
+ _LIBCPP_ALWAYS_INLINE _LIBCPP_INLINE_VISIBILITY<br>
+ constexpr mutex() = default;<br>
+ mutex(const mutex&) = delete;<br>
+ mutex& operator=(const mutex&) = delete;<br>
+ ~mutex() noexcept;<br>
+};<br>
+<br>
+<br>
+mutex::~mutex() _NOEXCEPT<br>
+{<br>
+ __libcpp_mutex_destroy(&__m_);<br>
+}<br>
+<br>
+#endif // !_LIBCPP_HAS_NO_THREADS<br>
+_LIBCPP_END_NAMESPACE_STD<br>
+<br>
<br>
Modified: libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp?rev=365273&r1=365272&r2=365273&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp?rev=365273&r1=365272&r2=365273&view=diff</a><br>
==============================================================================<br>
--- libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp (original)<br>
+++ libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp Sat Jul 6 18:20:54 2019<br>
@@ -21,8 +21,8 @@<br>
<br>
int main(int, char**)<br>
{<br>
- static_assert(std::is_nothrow_default_constructible<std::mutex>::value, "");<br>
- std::mutex m;<br>
-<br>
+ static_assert(std::is_nothrow_default_constructible<std::mutex>::value, "");<br>
+ std::mutex m;<br>
+ ((void)m);<br>
return 0;<br>
}<br>
<br>
<br>
_______________________________________________<br>
libcxx-commits mailing list<br>
<a href="mailto:libcxx-commits@lists.llvm.org" target="_blank">libcxx-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-commits</a><br>
</blockquote></div>