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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Mike Crowe
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.

Mike Crowe (2):
  Add user-defined clock to libstdc++ condition_variable tests
  PR libstdc++/41861 Add full steady_clock support to condition_variable

 libstdc++-v3/ChangeLog                                                | 34 +++++++++++++++++++++++++++++-
 libstdc++-v3/acinclude.m4                                             | 31 +++++++++++++++++++++++++++-
 libstdc++-v3/config.h.in                                              |  3 +++-
 libstdc++-v3/configure                                                | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 libstdc++-v3/configure.ac                                             |  3 +++-
 libstdc++-v3/include/std/condition_variable                           | 52 +++++++++++++++++++++++++++++++++++++++------
 libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc     | 29 ++++++++++++++++++++++---
 libstdc++-v3/testsuite/30_threads/condition_variable_any/members/2.cc | 29 ++++++++++++++++++++++---
 8 files changed, 252 insertions(+), 12 deletions(-)

base-commit: 7b0d7c79224b412ea5073593943ab1525cbbda73
--
git-series 0.9.1
Reply | Threaded
Open this post in threaded view
|

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

Jonathan Wakely-3
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?


patch.txt (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Mike Crowe
On Wednesday 04 September 2019 at 14:39:35 +0100, Jonathan Wakely wrote:

> 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?

You're far more expert on that stuff than I am, but it looks fine to me.

However, you've also removed the <bits/c++config.h> include. Was that
intentional?

> 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 found that it was necessary to run the test under strace to prove that
the correct variant of futex is called too, because...

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

Unfortunately, yes. CLOCK_MONOTONIC and CLOCK_REALTIME will tick at the
same rate, so unless someone warps CLOCK_REALTIME during the test we can't
tell the difference between the old implementation of translating
steady_clock to system_clock and the new implementation that uses
steady_clock directly. :( I added the tests in the hope of finding other
mistakes in the new implementation rather than to reproduce the original
problem.

Maybe I should add comments to the test to make that clear along the lines
of those found in testsuite/27_io/objects/char/3_xin.cc ?

Thanks for looking at this.

Mike.
Reply | Threaded
Open this post in threaded view
|

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

Mike Crowe
On Wednesday 04 September 2019 at 17:14:30 +0100, Jonathan Wakely wrote:

> On 04/09/19 15:49 +0100, Mike Crowe wrote:
> > On Wednesday 04 September 2019 at 14:39:35 +0100, Jonathan Wakely wrote:
> > > I noticed that the new tests you added in [PATCH 1/2] pass on current
> > > trunk, even without [PATCH 2/2], is that expected?
> >
> > Unfortunately, yes. CLOCK_MONOTONIC and CLOCK_REALTIME will tick at the
> > same rate, so unless someone warps CLOCK_REALTIME during the test we can't
> > tell the difference between the old implementation of translating
> > steady_clock to system_clock and the new implementation that uses
> > steady_clock directly. :( I added the tests in the hope of finding other
> > mistakes in the new implementation rather than to reproduce the original
> > problem.
>
> OK, that was what I figured was the case. Thanks for confirming.
>
> > Maybe I should add comments to the test to make that clear along the lines
> > of those found in testsuite/27_io/objects/char/3_xin.cc ?
>
> More comments are usually useful, otherwise I'll just ask the same
> question again and again :-)

How about something like:

--8<--
It's not possible for this test to automatically ensure that the
system_clock test cases result in a wait on CLOCK_REALTIME and steady_clock
test cases result in a wait on CLOCK_MONOTONIC. It's recommended to run the
test under strace(1) and check whether the expected futex calls are made by
glibc.
-->8--

Unfortunately I'm unable to determine how I actually managed to run the
test under strace. Perhaps I just compiled a similar test myself rather
than using dejagnu. :(

Mike.
Reply | Threaded
Open this post in threaded view
|

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

Mike Crowe
On Wednesday 04 September 2019 at 17:57:45 +0100, Mike Crowe wrote:

> On Wednesday 04 September 2019 at 17:14:30 +0100, Jonathan Wakely wrote:
> > On 04/09/19 15:49 +0100, Mike Crowe wrote:
> > > On Wednesday 04 September 2019 at 14:39:35 +0100, Jonathan Wakely wrote:
> > > > I noticed that the new tests you added in [PATCH 1/2] pass on current
> > > > trunk, even without [PATCH 2/2], is that expected?
> > >
> > > Unfortunately, yes. CLOCK_MONOTONIC and CLOCK_REALTIME will tick at the
> > > same rate, so unless someone warps CLOCK_REALTIME during the test we can't
> > > tell the difference between the old implementation of translating
> > > steady_clock to system_clock and the new implementation that uses
> > > steady_clock directly. :( I added the tests in the hope of finding other
> > > mistakes in the new implementation rather than to reproduce the original
> > > problem.
> >
> > OK, that was what I figured was the case. Thanks for confirming.
> >
> > > Maybe I should add comments to the test to make that clear along the lines
> > > of those found in testsuite/27_io/objects/char/3_xin.cc ?
> >
> > More comments are usually useful, otherwise I'll just ask the same
> > question again and again :-)
>
> How about something like:
>
> --8<--
> It's not possible for this test to automatically ensure that the
> system_clock test cases result in a wait on CLOCK_REALTIME and steady_clock
> test cases result in a wait on CLOCK_MONOTONIC. It's recommended to run the
> test under strace(1) and check whether the expected futex calls are made by
> glibc.
> -->8--
>
> Unfortunately I'm unable to determine how I actually managed to run the
> test under strace. Perhaps I just compiled a similar test myself rather
> than using dejagnu. :(

Ah, I did it. Here's a new comment:

--8<--
It's not possible for this test to automatically ensure that the
system_clock test cases result in a wait on CLOCK_REALTIME and steady_clock
test cases result in a wait on CLOCK_MONOTONIC. It's recommended to run the
test under strace(1) and check whether the expected futex calls are made by
glibc. The easiest way to do this is to copy and paste the line used to
build the test from the output of:

 make -j8 check-target-libstdc++-v3 RUNTESTFLAGS="conformance.exp=30_threads/condition_variable/members/* -v -v"

to compile the test with only the tests for one clock enabled and then run it as:

 strace ./2.exe

You should see calls to:

 futex(..., FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, ...)

with large values of tv_sec when using system_clock and calls to:

 futex(..., FUTEX_WAIT_BITSET_PRIVATE, ...)

with values of tv_sec based on the system uptime when using steady_clock.
-->8--

HTH.

Mike.