rmaprath added a comment. In http://reviews.llvm.org/D19412#417682, @EricWF wrote:
> In http://reviews.llvm.org/D19412#416596, @rmaprath wrote: > > > In http://reviews.llvm.org/D19412#416183, @EricWF wrote: > > > > > OK. I *think* this is my last round of review comments except for one > > > specific issue. I'm still looking into the changes to the static mutex's > > > and condition_variables in `memory.cpp` and `algorithm.cpp`. In these > > > cases I don't want to go from compile-time initialization to run-time > > > initialization. This could introduce the static initialization order > > > fiasco. > > > > > > So, as pointed out earlier by @bcraig, there won't be a runtime overhead > > for those compilers supporting `constexr`. For those that don't, this is > > still a trivial enough constructor call that is very likely to get > > optimized away. To elaborate, we can simplify this scenario to something > > like: > > > > **thread.h** > > > > struct thread_t { > > int state1; > > int *state2; > > bool state3; > > }; > > > > #define THREAD_INITIALIZER {0, 0, false} > > > > > > **test.cpp** > > > > #include "thread.h" > > > > class Foo { > > private: > > thread_t __t_; > > public: > > Foo() {__t_ = (thread_t) THREAD_INITIALIZER;} > > }; > > > > > Foo has a trivial destructor. Our mutex does not. I've already looked into > the codegen for clang and in each case your change creates runtime code. The > constructors may still be optimized away but it still has to register the > destructors. > > I'm just trying to figure out if that's going to be a problem. > > Example here: https://godbolt.org/g/dJL29v Hi Eric, Thanks for the pointer (and also for godbolt!). I can see that that my code creates runtime code to register the destructors. Apart from the overhead (registering the destructors at load time and then actually calling them when the DSO is unloaded), I can't see how this could lead to other issues. The ABI seem to be have a pretty well defined DSO destruction process [1]. Having to handle proper destruction of internal mutexes and condition variables sound like an OK thing for a standard library to do. But then, the following commit (which replaced `mutex` with the pthread native mutexes in `memory.cpp` originally) makes me think twice: commit e33c2d1926f49221c9d72a353d797d135a810d77 Author: Howard Hinnant <hhinn...@apple.com> Date: Sat Mar 16 00:17:53 2013 +0000 This should be nothing but a load-time optimization. I'm trying to reduce load time initializers and this is a big one. No visible functionality change intended. git-svn-id: https://llvm.org/svn/llvm-project/libcxx/trunk@177212 91177308-0d34-0410-b5e6-96231b3b80d8 diff --git a/src/memory.cpp b/src/memory.cpp index 14084a5..98bcc86 100644 --- a/src/memory.cpp +++ b/src/memory.cpp @@ -122,7 +122,15 @@ __shared_weak_count::__get_deleter(const type_info&) const _NOEXCEPT #if __has_feature(cxx_atomic) static const std::size_t __sp_mut_count = 16; -static mutex mut_back[__sp_mut_count]; +static pthread_mutex_t mut_back_imp[__sp_mut_count] = +{ + PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, + PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, + PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, + PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER +}; + +static mutex* mut_back = reinterpret_cast<std::mutex*>(mut_back_imp); _LIBCPP_CONSTEXPR __sp_mut::__sp_mut(void* p) _NOEXCEPT : __lx(p) So, perhaps it is best to leave these pthread mutexes alone, purely for performance reasons. We'll have to excuse a couple of `#ifdef _LIBCPP_THREAD_API_XXXX` conditionals in the library sources to allow the external threading API to function. Does that sound like an OK compromise? @EricWF, @mclow.lists? Thanks. / Asiri [1] https://mentorembedded.github.io/cxx-abi/abi.html#dso-dtor http://reviews.llvm.org/D19412 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
