[PATCH] D19412: [libcxx] Refactor pthread usage - II

Asiri Rathnayake via cfe-commits cfe-commits at lists.llvm.org
Sat Apr 30 01:21:38 PDT 2016


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 <hhinnant at 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





More information about the cfe-commits mailing list