* [PATCH] [libstdc++] Cleanup atomic timed wait implementation
@ 2021-06-04 22:29 Thomas Rodgers
2021-06-23 16:01 ` Jonathan Wakely
0 siblings, 1 reply; 2+ messages in thread
From: Thomas Rodgers @ 2021-06-04 22:29 UTC (permalink / raw)
To: gcc-patches, libstdc++; +Cc: trodgers, Thomas Rodgers
This cleans up the implementation of atomic_timed_wait.h and fixes the
accidental pessimization of spinning after waiting in
__timed_waiter_pool::_M_do_wait_until.
libstdc++-v3/ChangeLog:
* include/bits/atomic_timed_wait.h (__wait_clock_t): Define
conditionally.
(__cond_wait_until_impl): Define conditionally.
(__cond_wait_until): Define conditionally. Simplify clock
type detection/conversion.
(__timed_waiter_pool::_M_do_wait_until): Move the spin above
the wait.
---
libstdc++-v3/include/bits/atomic_timed_wait.h | 26 ++++++++++---------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h b/libstdc++-v3/include/bits/atomic_timed_wait.h
index ec7ff51cdbc..19386e5806a 100644
--- a/libstdc++-v3/include/bits/atomic_timed_wait.h
+++ b/libstdc++-v3/include/bits/atomic_timed_wait.h
@@ -51,7 +51,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
namespace __detail
{
+#ifdef _GLIBCXX_HAVE_LINUX_FUTEX || _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT
using __wait_clock_t = chrono::steady_clock;
+#else
+ using __wait_clock_t = chrono::system_clock;
+#endif
template<typename _Clock, typename _Dur>
__wait_clock_t::time_point
@@ -133,11 +137,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return false;
}
}
-#else
+// #elsif <some other platform mechanism,>
// define _GLIBCXX_HAVE_PLATFORM_TIMED_WAIT and implement __platform_wait_until()
// if there is a more efficient primitive supported by the platform
// (e.g. __ulock_wait())which is better than pthread_cond_clockwait
-#endif // ! PLATFORM_TIMED_WAIT
+#else
+// Use wait on condition variable
// Returns true if wait ended before timeout.
// _Clock must be either steady_clock or system_clock.
@@ -173,12 +178,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__cond_wait_until(__condvar& __cv, mutex& __mx,
const chrono::time_point<_Clock, _Dur>& __atime)
{
-#ifdef _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT
- if constexpr (is_same_v<_Clock, chrono::steady_clock>)
- return __detail::__cond_wait_until_impl(__cv, __mx, __atime);
- else
-#endif
- if constexpr (is_same_v<_Clock, chrono::system_clock>)
+ if constexpr (is_same_v<__wait_clock_t, _Clock>)
return __detail::__cond_wait_until_impl(__cv, __mx, __atime);
else
{
@@ -194,6 +194,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return false;
}
}
+#endif // ! PLATFORM_TIMED_WAIT
struct __timed_waiter_pool : __waiter_pool_base
{
@@ -300,17 +301,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
const chrono::time_point<_Clock, _Dur>&
__atime) noexcept
{
+
for (auto __now = _Clock::now(); __now < __atime;
__now = _Clock::now())
{
+ if (__base_type::_M_do_spin(__pred, __val,
+ __timed_backoff_spin_policy(__atime, __now)))
+ return true;
+
if (__base_type::_M_w._M_do_wait_until(
__base_type::_M_addr, __val, __atime)
&& __pred())
return true;
-
- if (__base_type::_M_do_spin(__pred, __val,
- __timed_backoff_spin_policy(__atime, __now)))
- return true;
}
return false;
}
--
2.26.2
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] [libstdc++] Cleanup atomic timed wait implementation
2021-06-04 22:29 [PATCH] [libstdc++] Cleanup atomic timed wait implementation Thomas Rodgers
@ 2021-06-23 16:01 ` Jonathan Wakely
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Wakely @ 2021-06-23 16:01 UTC (permalink / raw)
To: Thomas Rodgers; +Cc: gcc Patches, libstdc++, Thomas Rodgers
On Fri, 4 Jun 2021 at 23:31, Thomas Rodgers <rodgert@appliantology.com> wrote:
>
> This cleans up the implementation of atomic_timed_wait.h and fixes the
> accidental pessimization of spinning after waiting in
> __timed_waiter_pool::_M_do_wait_until.
This one's still pending review, right?
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/atomic_timed_wait.h (__wait_clock_t): Define
> conditionally.
> (__cond_wait_until_impl): Define conditionally.
> (__cond_wait_until): Define conditionally. Simplify clock
> type detection/conversion.
> (__timed_waiter_pool::_M_do_wait_until): Move the spin above
> the wait.
>
> ---
> libstdc++-v3/include/bits/atomic_timed_wait.h | 26 ++++++++++---------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h b/libstdc++-v3/include/bits/atomic_timed_wait.h
> index ec7ff51cdbc..19386e5806a 100644
> --- a/libstdc++-v3/include/bits/atomic_timed_wait.h
> +++ b/libstdc++-v3/include/bits/atomic_timed_wait.h
> @@ -51,7 +51,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> namespace __detail
> {
> +#ifdef _GLIBCXX_HAVE_LINUX_FUTEX || _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT
> using __wait_clock_t = chrono::steady_clock;
> +#else
> + using __wait_clock_t = chrono::system_clock;
> +#endif
>
> template<typename _Clock, typename _Dur>
> __wait_clock_t::time_point
> @@ -133,11 +137,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> return false;
> }
> }
> -#else
> +// #elsif <some other platform mechanism,>
"elsif" should be "elif" in this comment.
> // define _GLIBCXX_HAVE_PLATFORM_TIMED_WAIT and implement __platform_wait_until()
> // if there is a more efficient primitive supported by the platform
> // (e.g. __ulock_wait())which is better than pthread_cond_clockwait
> -#endif // ! PLATFORM_TIMED_WAIT
> +#else
> +// Use wait on condition variable
>
> // Returns true if wait ended before timeout.
> // _Clock must be either steady_clock or system_clock.
> @@ -173,12 +178,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> __cond_wait_until(__condvar& __cv, mutex& __mx,
> const chrono::time_point<_Clock, _Dur>& __atime)
> {
> -#ifdef _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT
> - if constexpr (is_same_v<_Clock, chrono::steady_clock>)
> - return __detail::__cond_wait_until_impl(__cv, __mx, __atime);
> - else
> -#endif
> - if constexpr (is_same_v<_Clock, chrono::system_clock>)
> + if constexpr (is_same_v<__wait_clock_t, _Clock>)
This doesn't seem quite right. __cond_wait_until_impl can always
accept time points using system_clock, even when __wait_clock is
steady_clock. With this change a system_clock timeout will be
converted to steady_clock if pthread_cond_clockwait is available. That
will happen even though __cond_wait_until_impl can always work with a
system_clock timeout. Is that what's intended?
If somebody calls try_acquire_until(system_clock::now() + 1s) then
they're asking to wait on the system clock, right? So the underlying
wait on a condition variable should use that clock if possible, right?
But when pthread_cond_clockwait is available, all absolute time points
are converted to steady_clock, even though system_clock is also
supported.
> return __detail::__cond_wait_until_impl(__cv, __mx, __atime);
> else
> {
> @@ -194,6 +194,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> return false;
> }
> }
> +#endif // ! PLATFORM_TIMED_WAIT
>
> struct __timed_waiter_pool : __waiter_pool_base
> {
> @@ -300,17 +301,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> const chrono::time_point<_Clock, _Dur>&
> __atime) noexcept
> {
> +
> for (auto __now = _Clock::now(); __now < __atime;
> __now = _Clock::now())
> {
> + if (__base_type::_M_do_spin(__pred, __val,
> + __timed_backoff_spin_policy(__atime, __now)))
> + return true;
> +
> if (__base_type::_M_w._M_do_wait_until(
> __base_type::_M_addr, __val, __atime)
> && __pred())
> return true;
> -
> - if (__base_type::_M_do_spin(__pred, __val,
> - __timed_backoff_spin_policy(__atime, __now)))
> - return true;
> }
> return false;
> }
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-06-23 16:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-04 22:29 [PATCH] [libstdc++] Cleanup atomic timed wait implementation Thomas Rodgers
2021-06-23 16:01 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).