[libstdc++] Cleanup atomic timed wait implementation

Message ID 20210604222912.815996-1-rodgert@appliantology.com
State New
Headers show
Series
  • [libstdc++] Cleanup atomic timed wait implementation
Related show

Commit Message

Thomas Rodgers June 4, 2021, 10:29 p.m.
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(-)

-- 
2.26.2

Comments

Jonathan Wakely via Gcc-patches June 23, 2021, 4:01 p.m. | #1
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

>

Patch

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;
 	  }