[libstdc++] Cleanup atomic timed wait implementation

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(-)

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
>