On Fri, 4 Jun 2021 at 23:31, Thomas Rodgers <rodg...@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
>
