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
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();
`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 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 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 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#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 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();
+int ec = __libcpp_recursive_mutex_init(&__m_);
if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-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();
+unique_lock lk(mut);
while (flag == 1)
-pthread_cond_wait(, );
+cv.wait(lk);
if (flag == 0)
{
#ifndef _LIBCPP_NO_EXCEPTIONS
try
{
#endif // _LIBCPP_NO_EXCEPTIONS
__libcpp_relaxed_store(, 1ul);
-pthread_mutex_unlock();
+lk.unlock();
func(arg);
-pthread_mutex_lock();
+lk.lock();
__libcpp_relaxed_store(, ~0ul);
-pthread_mutex_unlock();
-pthread_cond_broadcast();
+
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();
+int ec = __libcpp_recursive_mutex_init(&__m_);
if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-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();
+unique_lock lk(mut);
while (flag == 1)
-pthread_cond_wait(, );
+cv.wait(lk);
if (flag == 0)
{
#ifndef _LIBCPP_NO_EXCEPTIONS
try
{
#endif // _LIBCPP_NO_EXCEPTIONS
__libcpp_relaxed_store(, 1ul);
-pthread_mutex_unlock();
+lk.unlock();
func(arg);
-pthread_mutex_lock();
+lk.lock();
__libcpp_relaxed_store(, ~0ul);
-pthread_mutex_unlock();
-
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();
+int ec = __libcpp_recursive_mutex_init(&__m_);
if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-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();
+unique_lock lk(mut);
while (flag == 1)
-pthread_cond_wait(, );
+cv.wait(lk);
if (flag == 0)
{
#ifndef _LIBCPP_NO_EXCEPTIONS
try
{
#endif // _LIBCPP_NO_EXCEPTIONS
__libcpp_relaxed_store(, 1ul);
-pthread_mutex_unlock();
+lk.unlock();
func(arg);
-pthread_mutex_lock();
+lk.lock();
__libcpp_relaxed_store(, ~0ul);
-
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();
+int ec = __libcpp_recursive_mutex_init(&__m_);
if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-if (ec)
-{
-pthread_mutex_destroy(&__m_);
-
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();
+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();
+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 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();
+int ec = __libcpp_mutex_init(&__m_, true);
if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-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();
+unique_lock lk(mut);
while (flag == 1)
-pthread_cond_wait(, );
+cv.wait(lk);
if (flag == 0)
{
#ifndef _LIBCPP_NO_EXCEPTIONS
try
{
#endif // _LIBCPP_NO_EXCEPTIONS
__libcpp_relaxed_store(, 1ul);
-pthread_mutex_unlock();
+mut.unlock();
func(arg);
-pthread_mutex_lock();
+mut.lock();
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
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
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();
-if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
+int ec = __os_mutex_init(&__m_, true);
if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-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();
+unique_lock lk(mut);
while (flag == 1)
-pthread_cond_wait(, );
+cv.wait(lk);
if (flag == 0)
{
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 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();
-if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
+int ec = __os_mutex_init(&__m_, true);
if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-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();
+unique_lock lk(mut);
while (flag == 1)
-
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
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 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
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
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
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();
-if (ec)
-goto fail;
-ec = pthread_mutexattr_settype(, PTHREAD_MUTEX_RECURSIVE);
-if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutex_init(&__m_, );
+int ec = __os_mutex_init(&__m_, true);
if (ec)
-{
-pthread_mutexattr_destroy();
-goto fail;
-}
-ec = pthread_mutexattr_destroy();
-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.owns_lock() && (__count_ == 0
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 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();
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
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();
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) &&
[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();
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 =
