| Message ID | fc1cce85a6a8b4d39ff6826bea999edd2381911b.1590732962.git-series.mac@mcrowe.com |
|---|---|
| State | New |
| Headers | show |
| Series | std::future::wait_* and std::condition_variable improvements | expand |
On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote: >Convert the specified duration to the target clock's duration type >before adding it to the current time in >__atomic_futex_unsigned::_M_load_when_equal_for and >_M_load_when_equal_until. This removes the risk of the timeout being >rounded down to the current time resulting in there being no wait at >all when the duration type lacks sufficient precision to hold the >steady_clock current time. > >Rather than using the style of fix from PR68519, let's expose the C++17 >std::chrono::ceil function as std::chrono::__detail::ceil so that it can >be used in code compiled with earlier standards versions and simplify >the fix. This was suggested by John Salmon in >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91486#c5 . > >This problem has become considerably less likely to trigger since I >switched the __atomic__futex_unsigned::__clock_t reference clock from >system_clock to steady_clock and added the loop, but the consequences of >triggering it have changed too. > >By my calculations it takes just over 194 days from the epoch for the >current time not to be representable in a float. This means that >system_clock is always subject to the problem (with the standard 1970 >epoch) whereas steady_clock with float duration only runs out of >resolution machine has been running for that long (assuming the Linux >implementation of CLOCK_MONOTONIC.) > >The recently-added loop in >__atomic_futex_unsigned::_M_load_when_equal_until turns this scenario >into a busy wait. > >Unfortunately the combination of both of these things means that it's >not possible to write a test case for this occurring in >_M_load_when_equal_until as it stands. > > * libstdc++-v3/include/std/chrono: (__detail::ceil) Move > implementation of std::chrono::ceil into private namespace so > that it's available to pre-C++17 code. > > * libstdc++-v3/include/bits/atomic_futex.h: > (__atomic_futex_unsigned::_M_load_when_equal_for, > __atomic_futex_unsigned::_M_load_when_equal_until): Use > __detail::ceil to convert delta to the reference clock > duration type to avoid resolution problems > > * libstdc++-v3/testsuite/30_threads/async/async.cc: (test_pr91486): > New test for __atomic_futex_unsigned::_M_load_when_equal_for. >--- > libstdc++-v3/include/bits/atomic_futex.h | 6 +++-- > libstdc++-v3/include/std/chrono | 19 +++++++++++++---- > libstdc++-v3/testsuite/30_threads/async/async.cc | 15 +++++++++++++- > 3 files changed, 34 insertions(+), 6 deletions(-) > >diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h >index 5f95ade..aa137a7 100644 >--- a/libstdc++-v3/include/bits/atomic_futex.h >+++ b/libstdc++-v3/include/bits/atomic_futex.h >@@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _M_load_when_equal_for(unsigned __val, memory_order __mo, > const chrono::duration<_Rep, _Period>& __rtime) > { >+ using __dur = typename __clock_t::duration; > return _M_load_when_equal_until(__val, __mo, >- __clock_t::now() + __rtime); >+ __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime)); > } > > // Returns false iff a timeout occurred. >@@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > do { > const __clock_t::time_point __s_entry = __clock_t::now(); > const auto __delta = __atime - __c_entry; >- const auto __s_atime = __s_entry + __delta; >+ const auto __s_atime = __s_entry + >+ chrono::__detail::ceil<_Duration>(__delta); > if (_M_load_when_equal_until(__val, __mo, __s_atime)) > return true; > __c_entry = _Clock::now(); >diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono >index 6d78f32..4257c7c 100644 >--- a/libstdc++-v3/include/std/chrono >+++ b/libstdc++-v3/include/std/chrono >@@ -299,6 +299,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > #endif > #endif // C++20 > >+ // We want to use ceil even when compiling for earlier standards versions >+ namespace __detail >+ { >+ template<typename _ToDur, typename _Rep, typename _Period> >+ constexpr __enable_if_is_duration<_ToDur> >+ ceil(const duration<_Rep, _Period>& __d) >+ { >+ auto __to = chrono::duration_cast<_ToDur>(__d); >+ if (__to < __d) >+ return __to + _ToDur{1}; >+ return __to; >+ } >+ } >+ > #if __cplusplus >= 201703L > # define __cpp_lib_chrono 201611 > >@@ -316,10 +330,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > constexpr __enable_if_is_duration<_ToDur> > ceil(const duration<_Rep, _Period>& __d) > { >- auto __to = chrono::duration_cast<_ToDur>(__d); >- if (__to < __d) >- return __to + _ToDur{1}; >- return __to; >+ return __detail::ceil<_ToDur>(__d); This isn't valid in C++11, a constexpr function needs to be just a return statement. Fix incoming ...
On 11/09/20 15:41 +0100, Jonathan Wakely wrote: >On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote: >>Convert the specified duration to the target clock's duration type >>before adding it to the current time in >>__atomic_futex_unsigned::_M_load_when_equal_for and >>_M_load_when_equal_until. This removes the risk of the timeout being >>rounded down to the current time resulting in there being no wait at >>all when the duration type lacks sufficient precision to hold the >>steady_clock current time. >> >>Rather than using the style of fix from PR68519, let's expose the C++17 >>std::chrono::ceil function as std::chrono::__detail::ceil so that it can >>be used in code compiled with earlier standards versions and simplify >>the fix. This was suggested by John Salmon in >>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91486#c5 . >> >>This problem has become considerably less likely to trigger since I >>switched the __atomic__futex_unsigned::__clock_t reference clock from >>system_clock to steady_clock and added the loop, but the consequences of >>triggering it have changed too. >> >>By my calculations it takes just over 194 days from the epoch for the >>current time not to be representable in a float. This means that >>system_clock is always subject to the problem (with the standard 1970 >>epoch) whereas steady_clock with float duration only runs out of >>resolution machine has been running for that long (assuming the Linux >>implementation of CLOCK_MONOTONIC.) >> >>The recently-added loop in >>__atomic_futex_unsigned::_M_load_when_equal_until turns this scenario >>into a busy wait. >> >>Unfortunately the combination of both of these things means that it's >>not possible to write a test case for this occurring in >>_M_load_when_equal_until as it stands. >> >> * libstdc++-v3/include/std/chrono: (__detail::ceil) Move >> implementation of std::chrono::ceil into private namespace so >> that it's available to pre-C++17 code. >> >> * libstdc++-v3/include/bits/atomic_futex.h: >> (__atomic_futex_unsigned::_M_load_when_equal_for, >> __atomic_futex_unsigned::_M_load_when_equal_until): Use >> __detail::ceil to convert delta to the reference clock >> duration type to avoid resolution problems >> >> * libstdc++-v3/testsuite/30_threads/async/async.cc: (test_pr91486): >> New test for __atomic_futex_unsigned::_M_load_when_equal_for. >>--- >>libstdc++-v3/include/bits/atomic_futex.h | 6 +++-- >>libstdc++-v3/include/std/chrono | 19 +++++++++++++---- >>libstdc++-v3/testsuite/30_threads/async/async.cc | 15 +++++++++++++- >>3 files changed, 34 insertions(+), 6 deletions(-) >> >>diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h >>index 5f95ade..aa137a7 100644 >>--- a/libstdc++-v3/include/bits/atomic_futex.h >>+++ b/libstdc++-v3/include/bits/atomic_futex.h >>@@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> _M_load_when_equal_for(unsigned __val, memory_order __mo, >> const chrono::duration<_Rep, _Period>& __rtime) >> { >>+ using __dur = typename __clock_t::duration; >> return _M_load_when_equal_until(__val, __mo, >>- __clock_t::now() + __rtime); >>+ __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime)); >> } >> >> // Returns false iff a timeout occurred. >>@@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> do { >> const __clock_t::time_point __s_entry = __clock_t::now(); >> const auto __delta = __atime - __c_entry; >>- const auto __s_atime = __s_entry + __delta; >>+ const auto __s_atime = __s_entry + >>+ chrono::__detail::ceil<_Duration>(__delta); I'm testing the attached patch to fix the C++11 constexpr error, but while re-looking at the uses of __detail::ceil I noticed this is using _Duration as the target type. Shouldn't that be __clock_t::duration instead? Why do we care about the duration of the user's time_point here, rather than the preferred duration of the clock we're about to wait against? >> if (_M_load_when_equal_until(__val, __mo, __s_atime)) >> return true; >> __c_entry = _Clock::now(); >>diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono >>index 6d78f32..4257c7c 100644 >>--- a/libstdc++-v3/include/std/chrono >>+++ b/libstdc++-v3/include/std/chrono >>@@ -299,6 +299,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>#endif >>#endif // C++20 >> >>+ // We want to use ceil even when compiling for earlier standards versions >>+ namespace __detail >>+ { >>+ template<typename _ToDur, typename _Rep, typename _Period> >>+ constexpr __enable_if_is_duration<_ToDur> >>+ ceil(const duration<_Rep, _Period>& __d) >>+ { >>+ auto __to = chrono::duration_cast<_ToDur>(__d); >>+ if (__to < __d) >>+ return __to + _ToDur{1}; >>+ return __to; >>+ } >>+ } >>+ >>#if __cplusplus >= 201703L >># define __cpp_lib_chrono 201611 >> >>@@ -316,10 +330,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> constexpr __enable_if_is_duration<_ToDur> >> ceil(const duration<_Rep, _Period>& __d) >> { >>- auto __to = chrono::duration_cast<_ToDur>(__d); >>- if (__to < __d) >>- return __to + _ToDur{1}; >>- return __to; >>+ return __detail::ceil<_ToDur>(__d); > >This isn't valid in C++11, a constexpr function needs to be just a >return statement. Fix incoming ...
On 11/09/20 18:22 +0100, Jonathan Wakely wrote: >On 11/09/20 15:41 +0100, Jonathan Wakely wrote: >>On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote: >>>Convert the specified duration to the target clock's duration type >>>before adding it to the current time in >>>__atomic_futex_unsigned::_M_load_when_equal_for and >>>_M_load_when_equal_until. This removes the risk of the timeout being >>>rounded down to the current time resulting in there being no wait at >>>all when the duration type lacks sufficient precision to hold the >>>steady_clock current time. >>> >>>Rather than using the style of fix from PR68519, let's expose the C++17 >>>std::chrono::ceil function as std::chrono::__detail::ceil so that it can >>>be used in code compiled with earlier standards versions and simplify >>>the fix. This was suggested by John Salmon in >>>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91486#c5 . >>> >>>This problem has become considerably less likely to trigger since I >>>switched the __atomic__futex_unsigned::__clock_t reference clock from >>>system_clock to steady_clock and added the loop, but the consequences of >>>triggering it have changed too. >>> >>>By my calculations it takes just over 194 days from the epoch for the >>>current time not to be representable in a float. This means that >>>system_clock is always subject to the problem (with the standard 1970 >>>epoch) whereas steady_clock with float duration only runs out of >>>resolution machine has been running for that long (assuming the Linux >>>implementation of CLOCK_MONOTONIC.) >>> >>>The recently-added loop in >>>__atomic_futex_unsigned::_M_load_when_equal_until turns this scenario >>>into a busy wait. >>> >>>Unfortunately the combination of both of these things means that it's >>>not possible to write a test case for this occurring in >>>_M_load_when_equal_until as it stands. >>> >>> * libstdc++-v3/include/std/chrono: (__detail::ceil) Move >>> implementation of std::chrono::ceil into private namespace so >>> that it's available to pre-C++17 code. >>> >>> * libstdc++-v3/include/bits/atomic_futex.h: >>> (__atomic_futex_unsigned::_M_load_when_equal_for, >>> __atomic_futex_unsigned::_M_load_when_equal_until): Use >>> __detail::ceil to convert delta to the reference clock >>> duration type to avoid resolution problems >>> >>> * libstdc++-v3/testsuite/30_threads/async/async.cc: (test_pr91486): >>> New test for __atomic_futex_unsigned::_M_load_when_equal_for. >>>--- >>>libstdc++-v3/include/bits/atomic_futex.h | 6 +++-- >>>libstdc++-v3/include/std/chrono | 19 +++++++++++++---- >>>libstdc++-v3/testsuite/30_threads/async/async.cc | 15 +++++++++++++- >>>3 files changed, 34 insertions(+), 6 deletions(-) >>> >>>diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h >>>index 5f95ade..aa137a7 100644 >>>--- a/libstdc++-v3/include/bits/atomic_futex.h >>>+++ b/libstdc++-v3/include/bits/atomic_futex.h >>>@@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>> _M_load_when_equal_for(unsigned __val, memory_order __mo, >>> const chrono::duration<_Rep, _Period>& __rtime) >>> { >>>+ using __dur = typename __clock_t::duration; >>> return _M_load_when_equal_until(__val, __mo, >>>- __clock_t::now() + __rtime); >>>+ __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime)); >>> } >>> >>> // Returns false iff a timeout occurred. >>>@@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>> do { >>> const __clock_t::time_point __s_entry = __clock_t::now(); >>> const auto __delta = __atime - __c_entry; >>>- const auto __s_atime = __s_entry + __delta; >>>+ const auto __s_atime = __s_entry + >>>+ chrono::__detail::ceil<_Duration>(__delta); > >I'm testing the attached patch to fix the C++11 constexpr error, but >while re-looking at the uses of __detail::ceil I noticed this is using >_Duration as the target type. Shouldn't that be __clock_t::duration >instead? Why do we care about the duration of the user's time_point >here, rather than the preferred duration of the clock we're about to >wait against? > > >>> if (_M_load_when_equal_until(__val, __mo, __s_atime)) >>> return true; >>> __c_entry = _Clock::now(); >>>diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono >>>index 6d78f32..4257c7c 100644 >>>--- a/libstdc++-v3/include/std/chrono >>>+++ b/libstdc++-v3/include/std/chrono >>>@@ -299,6 +299,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>>#endif >>>#endif // C++20 >>> >>>+ // We want to use ceil even when compiling for earlier standards versions >>>+ namespace __detail >>>+ { >>>+ template<typename _ToDur, typename _Rep, typename _Period> >>>+ constexpr __enable_if_is_duration<_ToDur> >>>+ ceil(const duration<_Rep, _Period>& __d) >>>+ { >>>+ auto __to = chrono::duration_cast<_ToDur>(__d); >>>+ if (__to < __d) >>>+ return __to + _ToDur{1}; >>>+ return __to; >>>+ } >>>+ } >>>+ >>>#if __cplusplus >= 201703L >>># define __cpp_lib_chrono 201611 >>> >>>@@ -316,10 +330,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>> constexpr __enable_if_is_duration<_ToDur> >>> ceil(const duration<_Rep, _Period>& __d) >>> { >>>- auto __to = chrono::duration_cast<_ToDur>(__d); >>>- if (__to < __d) >>>- return __to + _ToDur{1}; >>>- return __to; >>>+ return __detail::ceil<_ToDur>(__d); >> >>This isn't valid in C++11, a constexpr function needs to be just a >>return statement. Fix incoming ... > > > >commit 2c56e931c694f98ba77c02163b69a62242f23a3b >Author: Jonathan Wakely <jwakely@redhat.com> >Date: Fri Sep 11 18:09:46 2020 > > libstdc++: Fix chrono::__detail::ceil to work with C++11 > > In C++11 constexpr functions can only have a return statement, so we > need to fix __detail::ceil to make it valid in C++11. This can be done > by moving the comparison and increment into a new function, __ceil_impl, > and calling that with the result of the duration_cast. > > This would mean the standard C++17 std::chrono::ceil function would make > two further calls, which would add too much overhead when not inlined. > For C++17 and later use a using-declaration to add chrono::ceil to > namespace __detail. For C++11 and C++14 define chrono::__detail::__ceil > as a C++11-compatible constexpr function template. > > libstdc++-v3/ChangeLog: > > * include/std/chrono [C++17] (chrono::__detail::ceil): Add > using declaration to make chrono::ceil available for internal > use with a consistent name. > (chrono::__detail::__ceil_impl): New function template. > (chrono::__detail::ceil): Use __ceil_impl to compare and > increment the value. Remove SFINAE constraint. > >diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono >index 893d1f6b2c9..2db2b7d0edc 100644 >--- a/libstdc++-v3/include/std/chrono >+++ b/libstdc++-v3/include/std/chrono >@@ -329,20 +329,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > #endif > #endif // C++20 > >- // We want to use ceil even when compiling for earlier standards versions >- namespace __detail >- { >- template<typename _ToDur, typename _Rep, typename _Period> >- constexpr __enable_if_is_duration<_ToDur> >- ceil(const duration<_Rep, _Period>& __d) >- { >- auto __to = chrono::duration_cast<_ToDur>(__d); >- if (__to < __d) >- return __to + _ToDur{1}; >- return __to; >- } >- } >- > #if __cplusplus >= 201703L > # define __cpp_lib_chrono 201611 > >@@ -360,7 +346,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > constexpr __enable_if_is_duration<_ToDur> > ceil(const duration<_Rep, _Period>& __d) > { >- return __detail::ceil<_ToDur>(__d); >+ auto __to = chrono::duration_cast<_ToDur>(__d); >+ if (__to < __d) >+ return __to - _ToDur{1}; Oops! That should be + not - Committed with that fix.
On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote: > > > diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h > > > index 5f95ade..aa137a7 100644 > > > --- a/libstdc++-v3/include/bits/atomic_futex.h > > > +++ b/libstdc++-v3/include/bits/atomic_futex.h > > > @@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > _M_load_when_equal_for(unsigned __val, memory_order __mo, > > > const chrono::duration<_Rep, _Period>& __rtime) > > > { > > > + using __dur = typename __clock_t::duration; > > > return _M_load_when_equal_until(__val, __mo, > > > - __clock_t::now() + __rtime); > > > + __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime)); > > > } > > > > > > // Returns false iff a timeout occurred. > > > @@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > do { > > > const __clock_t::time_point __s_entry = __clock_t::now(); > > > const auto __delta = __atime - __c_entry; > > > - const auto __s_atime = __s_entry + __delta; > > > + const auto __s_atime = __s_entry + > > > + chrono::__detail::ceil<_Duration>(__delta); On Friday 11 September 2020 at 18:22:04 +0100, Jonathan Wakely wrote: > I'm testing the attached patch to fix the C++11 constexpr error, but > while re-looking at the uses of __detail::ceil I noticed this is using > _Duration as the target type. Shouldn't that be __clock_t::duration > instead? Why do we care about the duration of the user's time_point > here, rather than the preferred duration of the clock we're about to > wait against? I think you're right. I've attached a patch to fix it and also add a test that would have failed at least some of the time if run on a machine with an uptime greater than 208.5 days with: void test_pr91486_wait_until(): Assertion 'float_steady_clock::call_count <= 3' failed. If we implement the optimisation to not re-check against the custom clock when the wait is complete if is_steady == true then the test would have started failing due to the wait not being long enough. (I used a couple of the GCC farm machines that have high uptimes to test this.) Thanks. Mike.
On 19/09/20 11:50 +0100, Mike Crowe wrote: >On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote: >> > > diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h >> > > index 5f95ade..aa137a7 100644 >> > > --- a/libstdc++-v3/include/bits/atomic_futex.h >> > > +++ b/libstdc++-v3/include/bits/atomic_futex.h >> > > @@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> > > _M_load_when_equal_for(unsigned __val, memory_order __mo, >> > > const chrono::duration<_Rep, _Period>& __rtime) >> > > { >> > > + using __dur = typename __clock_t::duration; >> > > return _M_load_when_equal_until(__val, __mo, >> > > - __clock_t::now() + __rtime); >> > > + __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime)); >> > > } >> > > >> > > // Returns false iff a timeout occurred. >> > > @@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> > > do { >> > > const __clock_t::time_point __s_entry = __clock_t::now(); >> > > const auto __delta = __atime - __c_entry; >> > > - const auto __s_atime = __s_entry + __delta; >> > > + const auto __s_atime = __s_entry + >> > > + chrono::__detail::ceil<_Duration>(__delta); > >On Friday 11 September 2020 at 18:22:04 +0100, Jonathan Wakely wrote: >> I'm testing the attached patch to fix the C++11 constexpr error, but >> while re-looking at the uses of __detail::ceil I noticed this is using >> _Duration as the target type. Shouldn't that be __clock_t::duration >> instead? Why do we care about the duration of the user's time_point >> here, rather than the preferred duration of the clock we're about to >> wait against? > >I think you're right. I've attached a patch to fix it and also add a test >that would have failed at least some of the time if run on a machine with >an uptime greater than 208.5 days with: > > void test_pr91486_wait_until(): Assertion 'float_steady_clock::call_count <= 3' failed. > >If we implement the optimisation to not re-check against the custom clock >when the wait is complete if is_steady == true then the test would have >started failing due to the wait not being long enough. > >(I used a couple of the GCC farm machines that have high uptimes to test >this.) Also pushed to master. Thanks! >Thanks. > >Mike. >From fa4decc00698785fb9e07aa36c0d862414ca5ff9 Mon Sep 17 00:00:00 2001 >From: Mike Crowe <mac@mcrowe.com> >Date: Wed, 16 Sep 2020 16:55:11 +0100 >Subject: [PATCH 2/2] libstdc++: Use correct duration for atomic_futex wait on > custom clock [PR 91486] > >As Jonathan Wakely pointed out[1], my change in commit >f9ddb696a289cc48d24d3d23c0b324cb88de9573 should have been rounding to >the target clock duration type rather than the input clock duration type >in __atomic_futex_unsigned::_M_load_when_equal_until just as (e.g.) >condition_variable does. > >As well as fixing this, let's create a rather contrived test that fails >with the previous code, but unfortunately only when run on a machine >with an uptime of over 208.5 days, and even then not always. > >[1] https://gcc.gnu.org/pipermail/libstdc++/2020-September/051004.html > >libstdc++-v3/ChangeLog: > > * include/bits/atomic_futex.h: > (__atomic_futex_unsigned::_M_load_when_equal_until): Use > target clock duration type when rounding. > > * testsuite/30_threads/async/async.cc: (test_pr91486_wait_for) > rename from test_pr91486. (float_steady_clock) new class for > test. (test_pr91486_wait_until) new test. >--- > libstdc++-v3/include/bits/atomic_futex.h | 2 +- > .../testsuite/30_threads/async/async.cc | 62 ++++++++++++++++++- > 2 files changed, 61 insertions(+), 3 deletions(-) > >diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h >index aa137a7b64e..6093be0fbc7 100644 >--- a/libstdc++-v3/include/bits/atomic_futex.h >+++ b/libstdc++-v3/include/bits/atomic_futex.h >@@ -235,7 +235,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > const __clock_t::time_point __s_entry = __clock_t::now(); > const auto __delta = __atime - __c_entry; > const auto __s_atime = __s_entry + >- chrono::__detail::ceil<_Duration>(__delta); >+ chrono::__detail::ceil<__clock_t::duration>(__delta); > if (_M_load_when_equal_until(__val, __mo, __s_atime)) > return true; > __c_entry = _Clock::now(); >diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc >index 46f8d2f327d..1c779bfbcad 100644 >--- a/libstdc++-v3/testsuite/30_threads/async/async.cc >+++ b/libstdc++-v3/testsuite/30_threads/async/async.cc >@@ -157,7 +157,7 @@ void test04() > } > } > >-void test_pr91486() >+void test_pr91486_wait_for() > { > future<void> f1 = async(launch::async, []() { > std::this_thread::sleep_for(std::chrono::seconds(1)); >@@ -171,6 +171,63 @@ void test_pr91486() > VERIFY( elapsed_steady >= std::chrono::seconds(1) ); > } > >+// This is a clock with a very recent epoch which ensures that the difference >+// between now() and one second in the future is representable in a float so >+// that when the generic clock version of >+// __atomic_futex_unsigned::_M_load_when_equal_until calculates the delta it >+// gets a duration of 1.0f. When chrono::steady_clock has moved sufficiently >+// far from its epoch (about 208.5 days in my testing - about 2^54ns because >+// there's a promotion to double happening too somewhere) adding 1.0f to the >+// current time has no effect. Using this clock ensures that >+// __atomic_futex_unsigned::_M_load_when_equal_until is using >+// chrono::__detail::ceil correctly so that the function actually sleeps rather >+// than spinning. >+struct float_steady_clock >+{ >+ using duration = std::chrono::duration<float>; >+ using rep = typename duration::rep; >+ using period = typename duration::period; >+ using time_point = std::chrono::time_point<float_steady_clock, duration>; >+ static constexpr bool is_steady = true; >+ >+ static chrono::steady_clock::time_point epoch; >+ static int call_count; >+ >+ static time_point now() >+ { >+ ++call_count; >+ auto real = std::chrono::steady_clock::now(); >+ return time_point{real - epoch}; >+ } >+}; >+ >+chrono::steady_clock::time_point float_steady_clock::epoch = chrono::steady_clock::now(); >+int float_steady_clock::call_count = 0; >+ >+void test_pr91486_wait_until() >+{ >+ future<void> f1 = async(launch::async, []() { >+ std::this_thread::sleep_for(std::chrono::seconds(1)); >+ }); >+ >+ float_steady_clock::time_point const now = float_steady_clock::now(); >+ >+ std::chrono::duration<float> const wait_time = std::chrono::seconds(1); >+ float_steady_clock::time_point const expire = now + wait_time; >+ VERIFY( expire > now ); >+ >+ auto const start_steady = chrono::steady_clock::now(); >+ auto status = f1.wait_until(expire); >+ auto const elapsed_steady = chrono::steady_clock::now() - start_steady; >+ >+ // This checks that we didn't come back too soon >+ VERIFY( elapsed_steady >= std::chrono::seconds(1) ); >+ >+ // This checks that wait_until didn't busy wait checking the clock more times >+ // than necessary. >+ VERIFY( float_steady_clock::call_count <= 3 ); >+} >+ > int main() > { > test01(); >@@ -179,6 +236,7 @@ int main() > test03<std::chrono::steady_clock>(); > test03<steady_clock_copy>(); > test04(); >- test_pr91486(); >+ test_pr91486_wait_for(); >+ test_pr91486_wait_until(); > return 0; > } >-- >2.28.0 >
diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h index 5f95ade..aa137a7 100644 --- a/libstdc++-v3/include/bits/atomic_futex.h +++ b/libstdc++-v3/include/bits/atomic_futex.h @@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_load_when_equal_for(unsigned __val, memory_order __mo, const chrono::duration<_Rep, _Period>& __rtime) { + using __dur = typename __clock_t::duration; return _M_load_when_equal_until(__val, __mo, - __clock_t::now() + __rtime); + __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime)); } // Returns false iff a timeout occurred. @@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION do { const __clock_t::time_point __s_entry = __clock_t::now(); const auto __delta = __atime - __c_entry; - const auto __s_atime = __s_entry + __delta; + const auto __s_atime = __s_entry + + chrono::__detail::ceil<_Duration>(__delta); if (_M_load_when_equal_until(__val, __mo, __s_atime)) return true; __c_entry = _Clock::now(); diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono index 6d78f32..4257c7c 100644 --- a/libstdc++-v3/include/std/chrono +++ b/libstdc++-v3/include/std/chrono @@ -299,6 +299,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #endif #endif // C++20 + // We want to use ceil even when compiling for earlier standards versions + namespace __detail + { + template<typename _ToDur, typename _Rep, typename _Period> + constexpr __enable_if_is_duration<_ToDur> + ceil(const duration<_Rep, _Period>& __d) + { + auto __to = chrono::duration_cast<_ToDur>(__d); + if (__to < __d) + return __to + _ToDur{1}; + return __to; + } + } + #if __cplusplus >= 201703L # define __cpp_lib_chrono 201611 @@ -316,10 +330,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION constexpr __enable_if_is_duration<_ToDur> ceil(const duration<_Rep, _Period>& __d) { - auto __to = chrono::duration_cast<_ToDur>(__d); - if (__to < __d) - return __to + _ToDur{1}; - return __to; + return __detail::ceil<_ToDur>(__d); } template <typename _ToDur, typename _Rep, typename _Period> diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc index ee117f4..f697292 100644 --- a/libstdc++-v3/testsuite/30_threads/async/async.cc +++ b/libstdc++-v3/testsuite/30_threads/async/async.cc @@ -158,6 +158,20 @@ void test04() } } +void test_pr91486() +{ + future<void> f1 = async(launch::async, []() { + std::this_thread::sleep_for(std::chrono::seconds(1)); + }); + + std::chrono::duration<float> const wait_time = std::chrono::seconds(1); + auto const start_steady = chrono::steady_clock::now(); + auto status = f1.wait_for(wait_time); + auto const elapsed_steady = chrono::steady_clock::now() - start_steady; + + VERIFY( elapsed_steady >= std::chrono::seconds(1) ); +} + int main() { test01(); @@ -166,5 +180,6 @@ int main() test03<std::chrono::steady_clock>(); test03<steady_clock_copy>(); test04(); + test_pr91486(); return 0; }
Convert the specified duration to the target clock's duration type before adding it to the current time in __atomic_futex_unsigned::_M_load_when_equal_for and _M_load_when_equal_until. This removes the risk of the timeout being rounded down to the current time resulting in there being no wait at all when the duration type lacks sufficient precision to hold the steady_clock current time. Rather than using the style of fix from PR68519, let's expose the C++17 std::chrono::ceil function as std::chrono::__detail::ceil so that it can be used in code compiled with earlier standards versions and simplify the fix. This was suggested by John Salmon in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91486#c5 . This problem has become considerably less likely to trigger since I switched the __atomic__futex_unsigned::__clock_t reference clock from system_clock to steady_clock and added the loop, but the consequences of triggering it have changed too. By my calculations it takes just over 194 days from the epoch for the current time not to be representable in a float. This means that system_clock is always subject to the problem (with the standard 1970 epoch) whereas steady_clock with float duration only runs out of resolution machine has been running for that long (assuming the Linux implementation of CLOCK_MONOTONIC.) The recently-added loop in __atomic_futex_unsigned::_M_load_when_equal_until turns this scenario into a busy wait. Unfortunately the combination of both of these things means that it's not possible to write a test case for this occurring in _M_load_when_equal_until as it stands. * libstdc++-v3/include/std/chrono: (__detail::ceil) Move implementation of std::chrono::ceil into private namespace so that it's available to pre-C++17 code. * libstdc++-v3/include/bits/atomic_futex.h: (__atomic_futex_unsigned::_M_load_when_equal_for, __atomic_futex_unsigned::_M_load_when_equal_until): Use __detail::ceil to convert delta to the reference clock duration type to avoid resolution problems * libstdc++-v3/testsuite/30_threads/async/async.cc: (test_pr91486): New test for __atomic_futex_unsigned::_M_load_when_equal_for. --- libstdc++-v3/include/bits/atomic_futex.h | 6 +++-- libstdc++-v3/include/std/chrono | 19 +++++++++++++---- libstdc++-v3/testsuite/30_threads/async/async.cc | 15 +++++++++++++- 3 files changed, 34 insertions(+), 6 deletions(-)