[PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath created this revision. rmaprath added reviewers: EricWF, theraven, mclow.lists, jroelofs. rmaprath added subscribers: espositofulvio, cfe-commits. This is mostly D11781 with the final review comments addressed: - Merged all the headers into a single `__os_support` header - Moved all internal functions (those only used by the library sources) away from the headers. This required adding a few `#ifdef`s into the library sources to select between the thread API. Note that this is only a refactoring, in that it isolates pthread usage of libcxx allowing anyone to easily plug-in a different thread implementation into libcxx sources. The final goal of this work (at least for us) is a bit more involved: we want to allow libcxx users to plug-in their own thread implementation at compile time. I have a patch for this which builds on the current one, I will be uploading it soon for comments. http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__os_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -37,6 +37,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD +using namespace __libcpp_os_support; + thread::~thread() { if (__t_ != 0) @@ -46,7 +48,11 @@ void thread::join() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_join(__t_, 0); +#else +#error "Not implemented for the selected thread API." +#endif #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +68,11 @@ int ec = EINVAL; if (__t_ != 0) { +#if defined(_LIBCPP_THREAD_API_PTHREAD) ec = pthread_detach(__t_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec == 0) __t_ = 0; } @@ -109,8 +119,7 @@ namespace this_thread { -void -sleep_for(const chrono::nanoseconds& ns) +void sleep_for(const chrono::nanoseconds& ns) { using namespace chrono; if (ns > nanoseconds::zero()) @@ -135,8 +144,7 @@ } } -} // this_thread - +} __thread_specific_ptr<__thread_struct>& __thread_local_data() { Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -21,15 +21,25 @@ const try_to_lock_t try_to_lock = {}; const adopt_lock_t adopt_lock = {}; +using namespace __libcpp_os_support; + mutex::~mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) pthread_mutex_destroy(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif } void mutex::lock() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_lock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec) __throw_system_error(ec, "mutex lock failed"); } @@ -37,13 +47,21 @@ bool mutex::try_lock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) return pthread_mutex_trylock(&__m_) == 0; +#else +#error "Not implemented for the selected thread API." +#endif } void mutex::unlock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_unlock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)ec; assert(ec == 0); } @@ -52,6 +70,7 @@ recursive_mutex::recursive_mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) pthread_mutexattr_t attr; int ec = pthread_mutexattr_init(&attr); if (ec) @@ -77,11 +96,18 @@ return; fail: __throw_system_error(ec, "recursive_mutex constructor failed"); +#else +#error "Not implemented for the selected thread API." +#endif } recursive_mutex::~recursive_mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int e = pthread_mutex_destroy(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)e; assert(e == 0); } @@ -89,7 +115,11 @@ void recursive_mutex::lock() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_lock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } @@ -97,7 +127,11 @@ void recursive_mutex::unlock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int e = pthread_mutex_unlock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)e; assert(e == 0); } @@ -105,7 +139,11 @@ bool recursive_mutex::try_lock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) return pthread_mutex_trylock(&__m_) == 0; +#else +#error "Not implemented for the selected thread API." +#endif } // timed_mutex @@ -165,9 +203,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pth
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 54658. rmaprath added a comment. Added bit more context to the diff. http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__os_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -37,6 +37,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD +using namespace __libcpp_os_support; + thread::~thread() { if (__t_ != 0) @@ -46,7 +48,11 @@ void thread::join() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_join(__t_, 0); +#else +#error "Not implemented for the selected thread API." +#endif #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +68,11 @@ int ec = EINVAL; if (__t_ != 0) { +#if defined(_LIBCPP_THREAD_API_PTHREAD) ec = pthread_detach(__t_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec == 0) __t_ = 0; } @@ -109,8 +119,7 @@ namespace this_thread { -void -sleep_for(const chrono::nanoseconds& ns) +void sleep_for(const chrono::nanoseconds& ns) { using namespace chrono; if (ns > nanoseconds::zero()) @@ -135,8 +144,7 @@ } } -} // this_thread - +} __thread_specific_ptr<__thread_struct>& __thread_local_data() { Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -21,37 +21,56 @@ const try_to_lock_t try_to_lock = {}; const adopt_lock_t adopt_lock = {}; +using namespace __libcpp_os_support; + mutex::~mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) pthread_mutex_destroy(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif } void mutex::lock() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_lock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) return pthread_mutex_trylock(&__m_) == 0; +#else +#error "Not implemented for the selected thread API." +#endif } void mutex::unlock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_unlock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) pthread_mutexattr_t attr; int ec = pthread_mutexattr_init(&attr); if (ec) @@ -77,35 +96,54 @@ return; fail: __throw_system_error(ec, "recursive_mutex constructor failed"); +#else +#error "Not implemented for the selected thread API." +#endif } recursive_mutex::~recursive_mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int e = pthread_mutex_destroy(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)e; assert(e == 0); } void recursive_mutex::lock() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_lock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int e = pthread_mutex_unlock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) return pthread_mutex_trylock(&__m_) == 0; +#else +#error "Not implemented for the selected thread API." +#endif } // timed_mutex @@ -165,9 +203,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__os_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__os_thread_id_compare(id, __id_) == 0) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +221,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__os_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __os_thread_id_compare(id, __id_) == 0)) { if (__count_ == numeric_limits::max()) return false; @@ -210,22 +248,17 @@ #endif // !_LIBCPP_HAS_NO_THREADS +#if !defined(_LIBCPP_HAS_NO_THREADS) && defin
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 54999. rmaprath added a comment. Minor cleanup + Gentle ping. http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__os_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -37,6 +37,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD +using namespace __libcpp_os_support; + thread::~thread() { if (__t_ != 0) @@ -46,7 +48,11 @@ void thread::join() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_join(__t_, 0); +#else +#error "Not implemented for the selected thread API." +#endif #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +68,11 @@ int ec = EINVAL; if (__t_ != 0) { +#if defined(_LIBCPP_THREAD_API_PTHREAD) ec = pthread_detach(__t_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -21,37 +21,56 @@ const try_to_lock_t try_to_lock = {}; const adopt_lock_t adopt_lock = {}; +using namespace __libcpp_os_support; + mutex::~mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) pthread_mutex_destroy(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif } void mutex::lock() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_lock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) return pthread_mutex_trylock(&__m_) == 0; +#else +#error "Not implemented for the selected thread API." +#endif } void mutex::unlock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_unlock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) pthread_mutexattr_t attr; int ec = pthread_mutexattr_init(&attr); if (ec) @@ -77,35 +96,54 @@ return; fail: __throw_system_error(ec, "recursive_mutex constructor failed"); +#else +#error "Not implemented for the selected thread API." +#endif } recursive_mutex::~recursive_mutex() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int e = pthread_mutex_destroy(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)e; assert(e == 0); } void recursive_mutex::lock() { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int ec = pthread_mutex_lock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) int e = pthread_mutex_unlock(&__m_); +#else +#error "Not implemented for the selected thread API." +#endif (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { +#if defined(_LIBCPP_THREAD_API_PTHREAD) return pthread_mutex_trylock(&__m_) == 0; +#else +#error "Not implemented for the selected thread API." +#endif } // timed_mutex @@ -165,9 +203,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__os_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__os_thread_id_compare(id, __id_) == 0) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +221,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__os_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __os_thread_id_compare(id, __id_) == 0)) { if (__count_ == numeric_limits::max()) return false; @@ -210,13 +248,12 @@ #endif // !_LIBCPP_HAS_NO_THREADS +#if !defined(_LIBCPP_HAS_NO_THREADS) && defined(_LIBCPP_THREAD_API_PTHREAD) // If dispatch_once_f ever handles C++ exceptions, and if one can get to it // without illegal macros (unexpected macros not beginning with _UpperCase or // __lowercase), and if it stops spinning waiting threads, then call_once should // call into dispatch_once_f instead of here. Relevant radar this code
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
bcraig added a subscriber: bcraig. Comment at: src/mutex.cpp:31 @@ +30,3 @@ +#else +#error "Not implemented for the selected thread API." +#endif Can't we just check for _LIBCPP_THREAD_API_PTHREAD once at the top of the file and #error as necessary there? I don't get the value of multiple #errors for the same condition, but sprinkled throughout the implementation. I do see the cost in the strategy, in that there are now lots of places that can bit-rot. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55119. rmaprath added a comment. Addressing review comments from @bcraig: In the earlier patch, I tried to keep the `__os_support` header to a minimum by not exposing the internal pthread dependencies (in library sources) in this header. This however blew up on me when externalizing the threading support in http://reviews.llvm.org/D19415, where I ended up sprinkling a lot of: #if defined(_LIBCPP_THREAD_API_PTHREAD) // Do pthread thing #elif defined(_LIBCPP_THREAD_API_EXTERNAL) // Do other thing #else // Fault #endif Perhaps the mistake was to view these two threading APIs as unrelated. After hiding all pthread dependencies behind the corresponding `__os_` functions (as suggested), everything falls into place (http://reviews.llvm.org/D19415 trivially becomes the list of `__os_` function declarations). I have addressed the remaining review comments along the way. Will update http://reviews.llvm.org/D19415 soon. @bcraig: Hope this is much better? http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__os_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -37,6 +37,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD +using namespace __libcpp_os_support; + thread::~thread() { if (__t_ != 0) @@ -46,7 +48,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __os_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +64,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __os_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -21,15 +21,17 @@ const try_to_lock_t try_to_lock = {}; const adopt_lock_t adopt_lock = {}; +using namespace __libcpp_os_support; + mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__os_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __os_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } @@ -37,13 +39,13 @@ bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __os_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __os_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } @@ -52,36 +54,14 @@ recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); -if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); +int ec = __os_mutex_init(&__m_, true); if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __os_mutex_destroy(&__m_); (void)e; assert(e == 0); } @@ -89,7 +69,7 @@ void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __os_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } @@ -97,7 +77,7 @@ void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __os_mutex_unlock(&__m_); (void)e; assert(e == 0); } @@ -105,7 +85,7 @@ bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __os_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +145,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__os_thread_id_compare(id, __id_) == 0) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +163,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.ow
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
bcraig added a comment. LGTM. You will still need to get approval from one of your original reviewers though. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
mclow.lists added a comment. In general, I'm happy with this direction. I think we need to check that there are no ABI changes that happen here (I don't see any, but I'll keep looking), and I think the stuff in `<__os_support>` should be marked "always inline" On a bikeshed note: is `<__os_support>` the right name? Or should it be something like `<__thread>` or `<__threading_support>`? Comment at: include/__os_support:32 @@ +31,3 @@ + +inline _LIBCPP_INLINE_VISIBILITY +int __os_mutex_init(__libcpp_mutex* __m, bool is_recursive = false) These should all be marked with `_LIBCPP_ALWAYS_INLINE`. Comment at: src/algorithm.cpp:51 @@ -50,3 +50,3 @@ #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t __rs_mut = PTHREAD_MUTEX_INITIALIZER; +static mutex __rs_mut; #endif What happened to the initializer here? Comment at: src/memory.cpp:130 @@ -129,11 +129,3 @@ static const std::size_t __sp_mut_count = 16; -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(mut_back_imp); +static mutex mut_back[__sp_mut_count]; How does this array get initialized now? Comment at: src/mutex.cpp:201 @@ -222,1 +200,3 @@ +static mutex mut; +static condition_variable cv; #endif These two variables used to be initialized. Now they're not. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath added a comment. > On a bikeshed note: is `<__os_support>` the right name? Or should it be > something like `<__thread>` or `<__threading_support>`? I went with `__os_support` in case if we'd want to group further external dependencies (like, for example, if some non-c POSIX calls are used in the upcoming `filesystem` library). I can revert back to `__threading_support` if that's preferred. Thanks. / Asiri Comment at: include/__os_support:32 @@ +31,3 @@ + +inline _LIBCPP_INLINE_VISIBILITY +int __os_mutex_init(__libcpp_mutex* __m, bool is_recursive = false) mclow.lists wrote: > These should all be marked with `_LIBCPP_ALWAYS_INLINE`. Got it. Will update. Comment at: src/algorithm.cpp:51 @@ -50,3 +50,3 @@ #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t __rs_mut = PTHREAD_MUTEX_INITIALIZER; +static mutex __rs_mut; #endif mclow.lists wrote: > What happened to the initializer here? I'm expecting the constructor of `mutex` to be run here at load time (before `main`). Note that this a libc++ mutex rather than a pthreads mutex (which does not required a constructor call like this). Would that be too much of an overhead? Comment at: src/memory.cpp:130 @@ -129,11 +129,3 @@ static const std::size_t __sp_mut_count = 16; -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(mut_back_imp); +static mutex mut_back[__sp_mut_count]; mclow.lists wrote: > How does this array get initialized now? Same as above, but bigger (this now requires 16 constructor calls during load time). I can check if clang would figure out that this is a trivial construction (at least for the default pthreads implementation) and be able to avoid the constructor calls altogether, but may be relying on such behaviour is not good. Comment at: src/mutex.cpp:201 @@ -222,1 +200,3 @@ +static mutex mut; +static condition_variable cv; #endif mclow.lists wrote: > These two variables used to be initialized. Now they're not. Same, relying on initialization at load time. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
bcraig added inline comments. Comment at: src/algorithm.cpp:51 @@ -50,3 +50,3 @@ #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t __rs_mut = PTHREAD_MUTEX_INITIALIZER; +static mutex __rs_mut; #endif rmaprath wrote: > mclow.lists wrote: > > What happened to the initializer here? > I'm expecting the constructor of `mutex` to be run here at load time (before > `main`). Note that this a libc++ mutex rather than a pthreads mutex (which > does not required a constructor call like this). Would that be too much of an > overhead? std::mutex's default ctor is constexpr. As long as the compiler supports constexpr, this initialization shouldn't require runtime code. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath added inline comments. Comment at: src/algorithm.cpp:51 @@ -50,3 +50,3 @@ #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t __rs_mut = PTHREAD_MUTEX_INITIALIZER; +static mutex __rs_mut; #endif bcraig wrote: > rmaprath wrote: > > mclow.lists wrote: > > > What happened to the initializer here? > > I'm expecting the constructor of `mutex` to be run here at load time > > (before `main`). Note that this a libc++ mutex rather than a pthreads mutex > > (which does not required a constructor call like this). Would that be too > > much of an overhead? > std::mutex's default ctor is constexpr. As long as the compiler supports > constexpr, this initialization shouldn't require runtime code. Missed that!. The same applied to std::condition_variable (used below). From the sources it looks like we do cater for compilers that do not support `constexpr`, so for those compilers it would depend on how clever they are in optimizing out this kind of trivial constructor calls. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55425. rmaprath added a comment. Addressing review comments by @mclow.lists: - Switched to `_LIBCPP_ALWAYS_INLINE` from `_LIBCPP_INLINE_VISIBILITY` for all `__os_xxx` functions. I've left the remaining points (naming of `__os_support` header and initialization of internal mutex / condition_variable instances) untouched pending further discussion. / Asiri http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__os_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -37,6 +37,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD +using namespace __libcpp_os_support; + thread::~thread() { if (__t_ != 0) @@ -46,7 +48,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __os_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +64,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __os_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -21,91 +21,71 @@ const try_to_lock_t try_to_lock = {}; const adopt_lock_t adopt_lock = {}; +using namespace __libcpp_os_support; + mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__os_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __os_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __os_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __os_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); -if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); +int ec = __os_mutex_init(&__m_, true); if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __os_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __os_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __os_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __os_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +145,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__os_thread_id_compare(id, __id_) == 0) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +163,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __os_thread_id_compare(id, __id_) == 0)) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +197,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static mutex mut; +static condition_variable cv; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +227,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +unique_lock lk(mut)
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
bcraig added a comment. In http://reviews.llvm.org/D19412#414374, @rmaprath wrote: > > On a bikeshed note: is `<__os_support>` the right name? Or should it be > > something like `<__thread>` or `<__threading_support>`? > > > I went with `__os_support` in case if we'd want to group further external > dependencies (like, for example, if some non-c POSIX calls are used in the > upcoming `filesystem` library). I can revert back to `__threading_support` if > that's preferred. > > Thanks. > > / Asiri I don't particularly care for <__os_support> either. Either of <__thread> or <__threading_support> are fine with me. Note that there are already other OS specific calls that libcxx has to handle. A lot of the locale code wants to use BSD style foo_l functions that are present on Linux. I prefer to keep the locale code independent of the threading code, as the two don't overlap much. For filesystem related items, I would expect those to go under <__filesystem> or <__filesystem_support>, or something similar. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55443. rmaprath added a comment. Renamed `__os_support` to `__threading_support` as suggested by @mclow.lists and @bcraig. I've left the namespace name `__libcpp_os_support` untouched, can change it to `__libcpp_threading_support` if the latter is preferred. Thanks! / Asiri http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -37,6 +37,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD +using namespace __libcpp_os_support; + thread::~thread() { if (__t_ != 0) @@ -46,7 +48,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __os_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +64,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __os_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -21,91 +21,71 @@ const try_to_lock_t try_to_lock = {}; const adopt_lock_t adopt_lock = {}; +using namespace __libcpp_os_support; + mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__os_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __os_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __os_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __os_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); -if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); +int ec = __os_mutex_init(&__m_, true); if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __os_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __os_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __os_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __os_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +145,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__os_thread_id_compare(id, __id_) == 0) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +163,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __os_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __os_thread_id_compare(id, __id_) == 0)) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +197,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static mutex mut; +static condition_variable cv; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +227,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +unique_lock lk(mut); while (flag == 1) -pthread_cond_wait(&cv, &mut); +cv.w
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
bcraig added a comment. LGTM. You'll still need to wait for one of the other reviewers though (probably @mclow.lists ) http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
EricWF added a comment. The patch looks good for the most part. Nit picking I would rather have `__libcpp_thread_foo` instead of `__libcpp_os_support::__os_thread_foo`. IMO the namespace is undeeded. @mclow.lists can you weigh in on this? Comment at: include/__mutex_base:43 @@ -43,3 +42,3 @@ #ifndef _LIBCPP_HAS_NO_CONSTEXPR - constexpr mutex() _NOEXCEPT : __m_(PTHREAD_MUTEX_INITIALIZER) {} +constexpr mutex() _NOEXCEPT : __m_(__OS_MUTEX_INITIALIZER) {} #else Similar to the reason we choose `__libcpp_mutex` these macros should have a `_LIBCPP` prefix as well instead of an `_OS` one. Comment at: include/__mutex_base:285 @@ -284,3 +284,3 @@ #else -condition_variable() {__cv_ = (pthread_cond_t)PTHREAD_COND_INITIALIZER;} +condition_variable() {__cv_ = (__libcpp_condvar)__OS_COND_INITIALIZER;} #endif Shouldn't this be `__cond_var_type`? http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55501. rmaprath added a comment. Addressing review comments from @EricWF: - Agree with all the suggested changes, I've applied them all. Thanks! / Asiri http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -46,7 +46,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __libcpp_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +62,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __libcpp_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -23,89 +23,67 @@ mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__libcpp_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); +int ec = __libcpp_mutex_init(&__m_, true); if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __libcpp_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __libcpp_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +143,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__libcpp_thread_id_compare(id, __id_) == 0) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +161,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __libcpp_thread_id_compare(id, __id_) == 0)) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +195,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static mutex mut; +static condition_variable cv; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +225,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +unique_lock lk(mut); while (flag == 1) -pthread_cond_wait(&cv, &mut); +cv.wait(lk); if (flag == 0) { #ifndef _LIBCPP_NO_EXCEPTIONS try { #endif // _LIBCPP_NO_EXCEPTIONS __libcpp_relaxed_store(&flag, 1ul); -pthread_mutex_unlock(&mut); +mut.unlock(); func(arg); -pthread_mutex_lock(&mut); +
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
EricWF added a comment. 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. Comment at: include/__mutex_base:37 @@ -38,2 +36,3 @@ { -pthread_mutex_t __m_; +typedef __libcpp_mutex_t __mutex_type; +__mutex_type __m_; Why not use `__libcpp_mutex_t` directly? Sorry for yet another renaming comment. Comment at: include/__mutex_base:43 @@ -43,3 +42,3 @@ #ifndef _LIBCPP_HAS_NO_CONSTEXPR - constexpr mutex() _NOEXCEPT : __m_(PTHREAD_MUTEX_INITIALIZER) {} +constexpr mutex() _NOEXCEPT : __m_(__LIBCPP_MUTEX_INITIALIZER) {} #else Only one underscore in `_LIBCPP_MUTEX_INITIALIZER` Comment at: include/__threading_support:11 @@ +10,3 @@ + +#ifndef _LIBCPP_OS_SUPPORT +#define _LIBCPP_OS_SUPPORT `_LIBCPP_THREADING_SUPPORT` Comment at: include/__threading_support:13 @@ +12,3 @@ +#define _LIBCPP_OS_SUPPORT + +#ifndef _LIBCPP_HAS_NO_THREADS ``` #include <__config> #ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER #pragma GCC system_header #endif ``` Comment at: include/__threading_support:30 @@ +29,3 @@ +inline _LIBCPP_ALWAYS_INLINE +int __libcpp_mutex_init(__libcpp_mutex_t* __m, bool is_recursive = false) +{ Two comments: 1. This is only used to initialize a recursive mutex. Let's simply name this `__libcpp_recursive_mutex_init` and get rid of the bool parameter. 2. This is only used in `recursive_mutex.cpp`. It's OK to keep this in `__threading_support` for now but I'll probably want to move this out of the headers later. Comment at: include/__threading_support:33 @@ +32,3 @@ +pthread_mutexattr_t attr; +int ec = pthread_mutexattr_init(&attr); +if (!ec && is_recursive) { Nit: Let's invert the control flow here: ``` if (ec) return ec; ec = pthread_mutexattr_settype(...); ``` Then at the end we can just `return 0;` Comment at: include/__threading_support:82 @@ +81,3 @@ +// Condition variable +#define __LIBCPP_COND_INITIALIZER PTHREAD_COND_INITIALIZER +typedef pthread_cond_t __libcpp_condvar_t; Nit: `_LIBCPP_CONDVAR_INITIALIZER` Comment at: include/__threading_support:119 @@ +118,3 @@ +inline _LIBCPP_ALWAYS_INLINE +int __libcpp_thread_id_compare(__libcpp_thread_id t1, __libcpp_thread_id t2) +{ Can we split this into `__libcpp_thread_id_equal` and `__libcpp_thread_id_less`? Comment at: include/__threading_support:129 @@ +128,3 @@ +inline _LIBCPP_ALWAYS_INLINE +int __libcpp_thread_create(__libcpp_thread_t* __t, _Func&& __f, _Arg&& __arg) +{ This signature needs to work in C++03. Let's make the signature `__libcpp_thread_create(__libcpp_thread_t* __t, _Func* __f, _Arg* __arg)` because both `_Func` and `_Arg` should be pointers so there is no need to perfect forward them. Comment at: include/__threading_support:169 @@ +168,3 @@ +inline _LIBCPP_ALWAYS_INLINE +int __libcpp_tl_create(__libcpp_tl_key* __key, _Func&& __at_exit) +{ This signature needs to work in C++03. Make the signature `__libcpp_tl_create(__libcpp_tl_key*, _Func*)` since the function must be a pointer. Comment at: include/__threading_support:194 @@ +193,2 @@ + +#endif // _LIBCPP_OS_SUPPORT `_LIBCPP_THREADING_SUPPORT` Comment at: src/mutex.cpp:228 @@ -249,3 +227,3 @@ #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +unique_lock lk(mut); while (flag == 1) This change seems incorrect. There are some paths below where we manually unlock `mut` before a `throw` or `return`. Using `unique_lock` will result in a double unlock. I think the fix is to change all `mut.lock()` and `mut.unlock()` calls into `lk.lock()`/`lk.unlock()` calls. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 7. rmaprath added a comment. 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 f(); int main() { return 0; } Compiling this with either `clang` or `gcc` in `-std=c++03` does **not** introduce an entry in the `.init_array` section. This can be observed with: > clang++ -std=c++03 -c test.cpp > objdump -j .init_array -dxs test.o On the other hand, if you slightly change the constructor to take a parameter, or print something into `std::cout`, there will be an entry in the `.init_array`. Moreover, even if some compiler naively emits a runtime initialization code for these constructors (`mutex` and `condition_variable`), I don't see how that can lead to problems related to static initialization order - all these constructors do is simply copy some memory, at worse, the compiler may emit a call to `memcpy` - but `memcpy` does not (AFAIK) depend on any other library state. In other words, these constructors don't require any other parts of the library having being initialized first. I hope that clears it? I have addressed the rest of you comments. Two minor nits: - Can we keep `__libcpp_mutex_t` (used in `__threading_support`) and `__mutex_type` (used in `__mutex_base`) separate? I feel it's cleaner to make a distinction between the types of the threading API and the rest of the library. I don't have a strong opinion though, let me know if you'd like it changed still. - I'd rather not move `__libcpp_recursive_mutex_init` back into `recursive_mutex.cpp`, as this would introduce pthread dependencies (`pthread_mutexattr_xxx`) back into the library sources. It's possible to workaround that by adding further `__libcpp_mutexattr_xxx` functions to the threading API, but those seem to be bit too pthread-specific. Again, no strong preference. Thanks! / Asiri http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -46,7 +46,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __libcpp_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +62,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __libcpp_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -23,13 +23,13 @@ mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__libcpp_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } @@ -37,13 +37,13 @@ bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } @@ -52,36 +52,14 @@ recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); +int ec = __libcpp_recursive_mutex_init(&__m_); if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthrea
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55569. rmaprath added a comment. Minor tweak: Got rid of the unnecessary template parameters on `__libcpp_thread_create` and `__libcpp_tl_create`. http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -46,7 +46,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __libcpp_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +62,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __libcpp_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -23,89 +23,67 @@ mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__libcpp_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); +int ec = __libcpp_recursive_mutex_init(&__m_); if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __libcpp_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __libcpp_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +143,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__libcpp_thread_id_equal(id, __id_)) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +161,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __libcpp_thread_id_equal(id, __id_))) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +195,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static mutex mut; +static condition_variable cv; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +225,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +unique_lock lk(mut); while (flag == 1) -pthread_cond_wait(&cv, &mut); +cv.wait(lk); if (flag == 0) { #ifndef _LIBCPP_NO_EXCEPTIONS try { #endif // _LIBCPP_NO_EXCEPTIONS __libcpp_relaxed_store(&flag, 1ul); -pthread_mutex_unlock(&mut); +lk.unlock(); func(arg); -pthread_mutex_lock(&mut); +lk.lock();
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55571. rmaprath added a comment. Missed one: s/_LIBCPP_COND_INITIALIZER/_LIBCPP_CONDVAR_INITIALIZER http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -46,7 +46,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __libcpp_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +62,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __libcpp_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -23,89 +23,67 @@ mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__libcpp_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); +int ec = __libcpp_recursive_mutex_init(&__m_); if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __libcpp_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __libcpp_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +143,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__libcpp_thread_id_equal(id, __id_)) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +161,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __libcpp_thread_id_equal(id, __id_))) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +195,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static mutex mut; +static condition_variable cv; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +225,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +unique_lock lk(mut); while (flag == 1) -pthread_cond_wait(&cv, &mut); +cv.wait(lk); if (flag == 0) { #ifndef _LIBCPP_NO_EXCEPTIONS try { #endif // _LIBCPP_NO_EXCEPTIONS __libcpp_relaxed_store(&flag, 1ul); -pthread_mutex_unlock(&mut); +lk.unlock(); func(arg); -pthread_mutex_lock(&mut); +lk.lock(); __libcpp_relaxed_store(&flag, ~0ul); -
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55593. rmaprath added a comment. Added missing `__confg` header include. http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -46,7 +46,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __libcpp_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +62,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __libcpp_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -23,89 +23,67 @@ mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__libcpp_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); +int ec = __libcpp_recursive_mutex_init(&__m_); if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __libcpp_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __libcpp_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +143,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__libcpp_thread_id_equal(id, __id_)) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +161,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __libcpp_thread_id_equal(id, __id_))) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +195,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static mutex mut; +static condition_variable cv; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +225,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +unique_lock lk(mut); while (flag == 1) -pthread_cond_wait(&cv, &mut); +cv.wait(lk); if (flag == 0) { #ifndef _LIBCPP_NO_EXCEPTIONS try { #endif // _LIBCPP_NO_EXCEPTIONS __libcpp_relaxed_store(&flag, 1ul); -pthread_mutex_unlock(&mut); +lk.unlock(); func(arg); -pthread_mutex_lock(&mut); +lk.lock(); __libcpp_relaxed_store(&flag, ~0ul); -pthread_mutex_u
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
EricWF added a comment. 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 http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
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 Date: Sat Mar 16 00:17:53 2013 + 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(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_` 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
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
EricWF added a comment. In http://reviews.llvm.org/D19412#417729, @rmaprath wrote: > 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_` conditionals in the library sources to allow the > external threading API to function. > > Does that sound like an OK compromise? @EricWF, @mclow.lists? Sounds good to me. Although we should probably change these from `pthread_mutex_t` to `__libcpp_mutex_t`. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55961. rmaprath added a comment. As agreed, reverted back to using the native mutex / condition_variable types within library sources. @EricWF: Good to go now? Thanks. / Asiri http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -46,7 +46,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __libcpp_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +62,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __libcpp_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -23,89 +23,67 @@ mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__libcpp_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); +int ec = __libcpp_recursive_mutex_init(&__m_); if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __libcpp_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __libcpp_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +143,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__libcpp_thread_id_equal(id, __id_)) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +161,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __libcpp_thread_id_equal(id, __id_))) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +195,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static __libcpp_mutex_t mut; +static __libcpp_condvar_t cv; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +225,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +__libcpp_mutex_lock(&mut); while (flag == 1) -pthread_cond_wait(&cv, &mut); +__libcpp_condvar_wait(&cv, &mut); if (flag == 0) { #ifndef _LIBCPP_NO_EXCEPTIONS try { #endif // _LIBCPP_NO_EXCEPTIONS __libcpp_relaxed_store(&flag, 1ul); -pthread_mutex_unlock(&mut); +__libcpp_mutex_unlock(&mut);
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath updated this revision to Diff 55965. rmaprath added a comment. Added missing initializer (typo). http://reviews.llvm.org/D19412 Files: include/__config include/__mutex_base include/__threading_support include/mutex include/thread src/algorithm.cpp src/condition_variable.cpp src/memory.cpp src/mutex.cpp src/thread.cpp Index: src/thread.cpp === --- src/thread.cpp +++ src/thread.cpp @@ -46,7 +46,7 @@ void thread::join() { -int ec = pthread_join(__t_, 0); +int ec = __libcpp_thread_join(&__t_); #ifndef _LIBCPP_NO_EXCEPTIONS if (ec) throw system_error(error_code(ec, system_category()), "thread::join failed"); @@ -62,7 +62,7 @@ int ec = EINVAL; if (__t_ != 0) { -ec = pthread_detach(__t_); +ec = __libcpp_thread_detach(&__t_); if (ec == 0) __t_ = 0; } Index: src/mutex.cpp === --- src/mutex.cpp +++ src/mutex.cpp @@ -23,89 +23,67 @@ mutex::~mutex() { -pthread_mutex_destroy(&__m_); +__libcpp_mutex_destroy(&__m_); } void mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "mutex lock failed"); } bool mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } void mutex::unlock() _NOEXCEPT { -int ec = pthread_mutex_unlock(&__m_); +int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } // recursive_mutex recursive_mutex::recursive_mutex() { -pthread_mutexattr_t attr; -int ec = pthread_mutexattr_init(&attr); +int ec = __libcpp_recursive_mutex_init(&__m_); if (ec) -goto fail; -ec = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutex_init(&__m_, &attr); -if (ec) -{ -pthread_mutexattr_destroy(&attr); -goto fail; -} -ec = pthread_mutexattr_destroy(&attr); -if (ec) -{ -pthread_mutex_destroy(&__m_); -goto fail; -} -return; -fail: -__throw_system_error(ec, "recursive_mutex constructor failed"); +__throw_system_error(ec, "recursive_mutex constructor failed"); } recursive_mutex::~recursive_mutex() { -int e = pthread_mutex_destroy(&__m_); +int e = __libcpp_mutex_destroy(&__m_); (void)e; assert(e == 0); } void recursive_mutex::lock() { -int ec = pthread_mutex_lock(&__m_); +int ec = __libcpp_mutex_lock(&__m_); if (ec) __throw_system_error(ec, "recursive_mutex lock failed"); } void recursive_mutex::unlock() _NOEXCEPT { -int e = pthread_mutex_unlock(&__m_); +int e = __libcpp_mutex_unlock(&__m_); (void)e; assert(e == 0); } bool recursive_mutex::try_lock() _NOEXCEPT { -return pthread_mutex_trylock(&__m_) == 0; +return __libcpp_mutex_trylock(&__m_) == 0; } // timed_mutex @@ -165,9 +143,9 @@ void recursive_timed_mutex::lock() { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_); -if (pthread_equal(id, __id_)) +if (__libcpp_thread_id_equal(id, __id_)) { if (__count_ == numeric_limits::max()) __throw_system_error(EAGAIN, "recursive_timed_mutex lock limit reached"); @@ -183,9 +161,9 @@ bool recursive_timed_mutex::try_lock() _NOEXCEPT { -pthread_t id = pthread_self(); +__libcpp_thread_id id = __libcpp_thread_get_current_id(); unique_lock lk(__m_, try_to_lock); -if (lk.owns_lock() && (__count_ == 0 || pthread_equal(id, __id_))) +if (lk.owns_lock() && (__count_ == 0 || __libcpp_thread_id_equal(id, __id_))) { if (__count_ == numeric_limits::max()) return false; @@ -217,8 +195,8 @@ // keep in sync with: 7741191. #ifndef _LIBCPP_HAS_NO_THREADS -static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static __libcpp_mutex_t mut = _LIBCPP_MUTEX_INITIALIZER; +static __libcpp_condvar_t cv = _LIBCPP_CONDVAR_INITIALIZER; #endif /// NOTE: Changes to flag are done via relaxed atomic stores @@ -247,36 +225,36 @@ #endif // _LIBCPP_NO_EXCEPTIONS } #else // !_LIBCPP_HAS_NO_THREADS -pthread_mutex_lock(&mut); +__libcpp_mutex_lock(&mut); while (flag == 1) -pthread_cond_wait(&cv, &mut); +__libcpp_condvar_wait(&cv, &mut); if (flag == 0) { #ifndef _LIBCPP_NO_EXCEPTIONS try { #endif // _LIBCPP_NO_EXCEPTIONS __libcpp_relaxed_store(&flag, 1ul); -pthread_mutex_unlock(&mut); +__libcpp_mutex_unlock(&mut); func(arg); -pthread_mutex_lock(&mu
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath added a comment. @mclow.lists, @EricWF: Gentle ping. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
EricWF accepted this revision. EricWF added a comment. This revision is now accepted and ready to land. LGTM after the minor fixes. Thank you for the patch. Comment at: include/__threading_support:38 @@ +37,3 @@ +{ +pthread_mutexattr_t attr; +int ec = pthread_mutexattr_init(&attr); `ec` -> `__ec`. Comment at: include/__threading_support:125 @@ +124,3 @@ +inline _LIBCPP_ALWAYS_INLINE +int __libcpp_thread_id_equal(__libcpp_thread_id t1, __libcpp_thread_id t2) +{ Let's make the return type bool so it's clear. Comment at: include/__threading_support:127 @@ +126,3 @@ +{ +return pthread_equal(t1, t2) != 0 ? 1 : 0; +} `return pthread_equal(t1, t2) != 0;` Comment at: include/__threading_support:132 @@ +131,3 @@ +inline _LIBCPP_ALWAYS_INLINE +int __libcpp_thread_id_less(__libcpp_thread_id t1, __libcpp_thread_id t2) +{ Make the return type `bool`. Comment at: include/__threading_support:134 @@ +133,3 @@ +{ +return t1 < t2 ? 1 : 0; +} `return t1 < t2; ` Comment at: include/thread:200 @@ -198,3 +199,3 @@ pointer __p = get(); -pthread_setspecific(__key_, 0); +__libcpp_tl_set(__key_, 0); return __p; Nit: Use `nullptr`. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19412: [libcxx] Refactor pthread usage - II
rmaprath closed this revision. rmaprath added a comment. @EricWF: Thanks for taking the time to review! :) Committed as r268734. The bots seem to be happy. http://reviews.llvm.org/D19412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits