This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/2] PR libstdc++/41861 Add full steady_clock support to condition_variable


On 15/07/19 17:47 +0100, Mike Crowe wrote:
The pthread_cond_clockwait function was recently added[1] to glibc, and is
due to be released in glibc 2.30. If this function is available in the C
library it can be used it to fix
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861 by supporting
std::chrono::steady_clock properly with std::condition_variable.

This means that code using std::condition_variable::wait_for or
std::condition_variable::wait_until with std::chrono::steady_clock is no
longer subject to timing out early or potentially waiting for much longer
if the system clock is warped at an inopportune moment.

If pthread_cond_clockwait is available then std::chrono::steady_clock is
deemed to be the "best" clock available which means that it is used for the
relative wait_for calls and absolute wait_until calls using user-defined
clocks. Calls explicitly using std::chrono::system_clock continue to use
CLOCK_REALTIME via __gthread_cond_timedwait.

If pthread_cond_clockwait is not available then std::chrono::system_clock
is deemed to be the "best" clock available which means that the previous
suboptimal behaviour remains.

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=afe4de7d283ebd88157126c5494ce1796194c16e

libstdc++-v3/

	* include/std/condition_variable: Add include of <bits/c++config.h>
	to make _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT available.
	(condition_variable): Split __clock_t into __system_clock_t, which
	is always system_clock and __best_clock_t, which is the clock that
	we wish to convert arbitrary other clocks to.  If we know that
	pthread_cond_clockwait is available then it is best to convert
	clocks to steady_clock, otherwise it's best to use
	system_clock. (wait_until): If pthread_cond_clockwait is available,
	provide a steady_clock overload.  Convert previous __clock_t
	overload to use __system_clock_t.  Convert generic overload to
	convert passed clock to __best_clock_t.  (wait_until_impl): Add
	steady_clock overload that calls pthread_cond_clockwait.  Convert
	previous __clock_t overload to use
	__system_clock_t. (condition_variable_any): Use steady_clock for
	__clock_t if pthread_cond_clockwait is available.

	* acinclude.m4:	Detect the availability of POSIX-proposed
	pthread_cond_clockwait function.
	* configure: Likewise.
	* configure.ac: Likewise.
	* config.h.in: Add _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT define to
	indicate presence of pthread_cond_clockwait function.

Thanks, Mike!

This patch is much simpler than the previous versions. Thanks for
perservering with POSIX and glibc to get the necessary functionality
into libc.

I've attached a slightly-modified patch, which changes the names of
the clock typedefs in std::condition_variable. The names
"steady_clock" and "system_clock" are already reserved names, so we
can just use those (instead of the uglier __steady_clock_t forms). And
we can just keep the original __clock_t name to mean the best clock.
Does that look OK to you too?

I've confirmed that with glibc 2.30 the new function is detected, and
the testsuite passes. I'm running the tests with an older glibc as
well.

I noticed that the new tests you added in [PATCH 1/2] pass on current
trunk, even without [PATCH 2/2], is that expected?

commit d3ddd730a7a2ca4a9d207eb3e1229f5cb8c652e4
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 4 12:59:04 2019 +0100

    PR libstdc++/41861 Add full steady_clock support to condition_variable
    
    The pthread_cond_clockwait function is available in glibc since the 2.30
    release. If this function is available in the C library it can be used
    to fix PR libstdc++/41861 by supporting std::chrono::steady_clock
    properly with std::condition_variable.
    
    This means that code using std::condition_variable::wait_for or
    std::condition_variable::wait_until with std::chrono::steady_clock is no
    longer subject to timing out early or potentially waiting for much
    longer if the system clock is warped at an inopportune moment.
    
    If pthread_cond_clockwait is available then std::chrono::steady_clock is
    deemed to be the "best" clock available which means that it is used for
    the relative wait_for calls and absolute wait_until calls using
    user-defined clocks. Calls explicitly using std::chrono::system_clock
    continue to use CLOCK_REALTIME via __gthread_cond_timedwait.
    
    If pthread_cond_clockwait is not available then
    std::chrono::system_clock is deemed to be the "best" clock available
    which means that the previous suboptimal behaviour remains.
    
    2019-09-04  Mike Crowe  <mac@mcrowe.com>
    
            PR libstdc++/41861
            * acinclude.m4 (GLIBCXX_CHECK_PTHREAD_COND_CLOCKWAIT): Check for new
            pthread_cond_clockwait function.
            * configure.ac: Use GLIBCXX_CHECK_PTHREAD_COND_CLOCKWAIT.
            * configure: Regenerate.
            * config.h.in: Regenerate.
            * include/std/condition_variable: (condition_variable): Rename
            __steady_clock_t typedef and add system_clock. Change __clock_t to be
            a typedef for the preferred clock to convert arbitrary other clocks to.
            [_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT] (wait_until): Add a steady_clock
            overload.
            (wait_until): Change __clock_t overload to use system_clock.
            [_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT] (__wait_until_impl): Add
            steady_clock overload that calls pthread_cond_clockwait.
            (__wait_until_impl): Change __clock_t overload to use system_clock.
            (condition_variable_any) [_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT]: Use
            steady_clock for __clock_t if pthread_cond_clockwait is available.

diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
index 727d6ce4239..bc9095f4799 100644
--- a/libstdc++-v3/acinclude.m4
+++ b/libstdc++-v3/acinclude.m4
@@ -4193,6 +4193,37 @@ AC_DEFUN([GLIBCXX_CHECK_PTHREADS_NUM_PROCESSORS_NP], [
   AC_LANG_RESTORE
 ])
 
+dnl
+dnl Check whether pthread_cond_clockwait is available in <pthread.h> for std::condition_variable to use,
+dnl and define _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT.
+dnl
+AC_DEFUN([GLIBCXX_CHECK_PTHREAD_COND_CLOCKWAIT], [
+
+  AC_LANG_SAVE
+  AC_LANG_CPLUSPLUS
+  ac_save_CXXFLAGS="$CXXFLAGS"
+  CXXFLAGS="$CXXFLAGS -fno-exceptions"
+  ac_save_LIBS="$LIBS"
+  LIBS="$LIBS -lpthread"
+
+  AC_MSG_CHECKING([for pthread_cond_clockwait])
+  AC_CACHE_VAL(glibcxx_cv_PTHREAD_COND_CLOCKWAIT, [
+    GCC_TRY_COMPILE_OR_LINK(
+      [#include <pthread.h>],
+      [pthread_mutex_t mutex; pthread_cond_t cond; struct timespec ts; int n = pthread_cond_clockwait(&cond, &mutex, 0, &ts);],
+      [glibcxx_cv_PTHREAD_COND_CLOCKWAIT=yes],
+      [glibcxx_cv_PTHREAD_COND_CLOCKWAIT=no])
+  ])
+  if test $glibcxx_cv_PTHREAD_COND_CLOCKWAIT = yes; then
+    AC_DEFINE(_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT, 1, [Define if pthread_cond_clockwait is available in <pthread.h>.])
+  fi
+  AC_MSG_RESULT($glibcxx_cv_PTHREAD_COND_CLOCKWAIT)
+
+  CXXFLAGS="$ac_save_CXXFLAGS"
+  LIBS="$ac_save_LIBS"
+  AC_LANG_RESTORE
+])
+
 dnl
 dnl Check whether sysctl is available in <pthread.h>, and define _GLIBCXX_USE_SYSCTL_HW_NCPU.
 dnl
diff --git a/libstdc++-v3/configure.ac b/libstdc++-v3/configure.ac
index 80d8202c337..ad4ae0c3b7d 100644
--- a/libstdc++-v3/configure.ac
+++ b/libstdc++-v3/configure.ac
@@ -220,6 +220,9 @@ GLIBCXX_ENABLE_LIBSTDCXX_TIME
 # Check for tmpnam which is obsolescent in POSIX.1-2008
 GLIBCXX_CHECK_TMPNAM
 
+# For pthread_cond_clockwait
+GLIBCXX_CHECK_PTHREAD_COND_CLOCKWAIT
+
 AC_LC_MESSAGES
 
 # For hardware_concurrency
diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index a83996aca3d..ff41548aed8 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -65,8 +65,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   /// condition_variable
   class condition_variable
   {
-    typedef chrono::system_clock	__clock_t;
-    typedef chrono::steady_clock	__steady_clock_t;
+    using steady_clock = chrono::steady_clock;
+    using system_clock = chrono::system_clock;
+#if defined(_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT)
+    using __clock_t = steady_clock;
+#else
+    using __clock_t = system_clock;
+#endif
     typedef __gthread_cond_t		__native_type;
 
 #ifdef __GTHREAD_COND_INIT
@@ -101,10 +106,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  wait(__lock);
       }
 
+#if defined(_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT)
     template<typename _Duration>
       cv_status
       wait_until(unique_lock<mutex>& __lock,
-		 const chrono::time_point<__clock_t, _Duration>& __atime)
+		 const chrono::time_point<steady_clock, _Duration>& __atime)
+      { return __wait_until_impl(__lock, __atime); }
+#endif
+
+    template<typename _Duration>
+      cv_status
+      wait_until(unique_lock<mutex>& __lock,
+		 const chrono::time_point<system_clock, _Duration>& __atime)
       { return __wait_until_impl(__lock, __atime); }
 
     template<typename _Clock, typename _Duration>
@@ -112,7 +125,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       wait_until(unique_lock<mutex>& __lock,
 		 const chrono::time_point<_Clock, _Duration>& __atime)
       {
-	// DR 887 - Sync unknown clock to known clock.
 	const typename _Clock::time_point __c_entry = _Clock::now();
 	const __clock_t::time_point __s_entry = __clock_t::now();
 	const auto __delta = __atime - __c_entry;
@@ -145,11 +157,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       wait_for(unique_lock<mutex>& __lock,
 	       const chrono::duration<_Rep, _Period>& __rtime)
       {
-	using __dur = typename __steady_clock_t::duration;
+	using __dur = typename steady_clock::duration;
 	auto __reltime = chrono::duration_cast<__dur>(__rtime);
 	if (__reltime < __rtime)
 	  ++__reltime;
-	return wait_until(__lock, __steady_clock_t::now() + __reltime);
+	return wait_until(__lock, steady_clock::now() + __reltime);
       }
 
     template<typename _Rep, typename _Period, typename _Predicate>
@@ -158,11 +170,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	       const chrono::duration<_Rep, _Period>& __rtime,
 	       _Predicate __p)
       {
-	using __dur = typename __steady_clock_t::duration;
+	using __dur = typename steady_clock::duration;
 	auto __reltime = chrono::duration_cast<__dur>(__rtime);
 	if (__reltime < __rtime)
 	  ++__reltime;
-	return wait_until(__lock, __steady_clock_t::now() + __reltime,
+	return wait_until(__lock, steady_clock::now() + __reltime,
 			  std::move(__p));
       }
 
@@ -171,10 +183,34 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     { return &_M_cond; }
 
   private:
+#if defined(_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT)
     template<typename _Dur>
       cv_status
       __wait_until_impl(unique_lock<mutex>& __lock,
-			const chrono::time_point<__clock_t, _Dur>& __atime)
+			const chrono::time_point<steady_clock, _Dur>& __atime)
+      {
+	auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
+	auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
+
+	__gthread_time_t __ts =
+	  {
+	    static_cast<std::time_t>(__s.time_since_epoch().count()),
+	    static_cast<long>(__ns.count())
+	  };
+
+	pthread_cond_clockwait(&_M_cond, __lock.mutex()->native_handle(),
+					 CLOCK_MONOTONIC,
+					 &__ts);
+
+	return (steady_clock::now() < __atime
+		? cv_status::no_timeout : cv_status::timeout);
+      }
+#endif
+
+    template<typename _Dur>
+      cv_status
+      __wait_until_impl(unique_lock<mutex>& __lock,
+			const chrono::time_point<system_clock, _Dur>& __atime)
       {
 	auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
 	auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
@@ -188,7 +224,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__gthread_cond_timedwait(&_M_cond, __lock.mutex()->native_handle(),
 				 &__ts);
 
-	return (__clock_t::now() < __atime
+	return (system_clock::now() < __atime
 		? cv_status::no_timeout : cv_status::timeout);
       }
   };
@@ -208,7 +244,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // Like above, but mutex is not required to have try_lock.
   class condition_variable_any
   {
-    typedef chrono::system_clock	__clock_t;
+#if defined(_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT)
+    using __clock_t = chrono::steady_clock;
+#else
+    using __clock_t = chrono::system_clock;
+#endif
     condition_variable			_M_cond;
     shared_ptr<mutex>			_M_mutex;
 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]