public inbox for libstdc++-cvs@sourceware.org help / color / mirror / Atom feed
From: Jonathan Wakely <redi@gcc.gnu.org> To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r11-3158] libstdc++: Avoid rounding errors on custom clocks in condition_variable Date: Fri, 11 Sep 2020 13:29:43 +0000 (GMT) [thread overview] Message-ID: <20200911132943.3F4B1398750C@sourceware.org> (raw) https://gcc.gnu.org/g:e05ff30078e80869f2bf3af6dbdbea134c252158 commit r11-3158-ge05ff30078e80869f2bf3af6dbdbea134c252158 Author: Mike Crowe <mac@mcrowe.com> Date: Fri Sep 11 14:25:00 2020 +0100 libstdc++: Avoid rounding errors on custom clocks in condition_variable The fix for PR68519 in 83fd5e73b3c16296e0d7ba54f6c547e01c7eae7b only applied to condition_variable::wait_for. This problem can also apply to condition_variable::wait_until but only if the custom clock is using a more recent epoch so that a small enough delta can be calculated. let's use the newly-added chrono::__detail::ceil to fix this and also make use of that function to simplify the previous wait_for fixes. Also, simplify the existing test case for PR68519 a little and make its variables local so we can add a new test case for the above problem. Unfortunately, the test would have only started failing if sufficient time has passed since the chrono::steady_clock epoch had passed anyway, but it's better than nothing. libstdc++-v3/ChangeLog: * include/std/condition_variable (condition_variable::wait_until): Convert delta to steady_clock duration before adding to current steady_clock time to avoid rounding errors described in PR68519. (condition_variable::wait_for): Simplify calculation of absolute time by using chrono::__detail::ceil in both overloads. * testsuite/30_threads/condition_variable/members/68519.cc: (test_wait_for): Renamed from test01. Replace unassigned val variable with constant false. Reduce scope of mx and cv variables to just test_wait_for function. (test_wait_until): Add new test case. Diff: --- libstdc++-v3/include/std/condition_variable | 18 +++---- .../30_threads/condition_variable/members/68519.cc | 61 +++++++++++++++++++--- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable index a08cfc62705..1abec546320 100644 --- a/libstdc++-v3/include/std/condition_variable +++ b/libstdc++-v3/include/std/condition_variable @@ -133,10 +133,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #if __cplusplus > 201703L static_assert(chrono::is_clock_v<_Clock>); #endif + using __s_dur = typename __clock_t::duration; 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; - const auto __s_atime = __s_entry + __delta; + const auto __s_atime = __s_entry + + chrono::__detail::ceil<__s_dur>(__delta); if (__wait_until_impl(__lock, __s_atime) == cv_status::no_timeout) return cv_status::no_timeout; @@ -166,10 +168,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const chrono::duration<_Rep, _Period>& __rtime) { using __dur = typename steady_clock::duration; - auto __reltime = chrono::duration_cast<__dur>(__rtime); - if (__reltime < __rtime) - ++__reltime; - return wait_until(__lock, steady_clock::now() + __reltime); + return wait_until(__lock, + steady_clock::now() + + chrono::__detail::ceil<__dur>(__rtime)); } template<typename _Rep, typename _Period, typename _Predicate> @@ -179,10 +180,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Predicate __p) { using __dur = typename steady_clock::duration; - auto __reltime = chrono::duration_cast<__dur>(__rtime); - if (__reltime < __rtime) - ++__reltime; - return wait_until(__lock, steady_clock::now() + __reltime, + return wait_until(__lock, + steady_clock::now() + + chrono::__detail::ceil<__dur>(__rtime), std::move(__p)); } diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc b/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc index 2a6ecb95fd1..c86ca2ccdbf 100644 --- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc +++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc @@ -25,25 +25,72 @@ // PR libstdc++/68519 -bool val = false; -std::mutex mx; -std::condition_variable cv; - void -test01() +test_wait_for() { + std::mutex mx; + std::condition_variable cv; + for (int i = 0; i < 3; ++i) { std::unique_lock<std::mutex> l(mx); auto start = std::chrono::system_clock::now(); - cv.wait_for(l, std::chrono::duration<float>(1), [] { return val; }); + cv.wait_for(l, std::chrono::duration<float>(1), [] { return false; }); auto t = std::chrono::system_clock::now(); VERIFY( (t - start) >= std::chrono::seconds(1) ); } } +// In order to ensure that the delta calculated in the arbitrary clock overload +// of condition_variable::wait_until fits accurately in a float, but the result +// of adding it to steady_clock with a float duration does not, this clock +// needs to use a more recent epoch. +struct recent_epoch_float_clock +{ + using rep = std::chrono::duration<float>::rep; + using period = std::chrono::duration<float>::period; + using time_point = std::chrono::time_point<recent_epoch_float_clock, + std::chrono::duration<float>>; + static constexpr bool is_steady = true; + + static const std::chrono::steady_clock::time_point epoch; + + static time_point now() + { + const auto steady = std::chrono::steady_clock::now(); + return time_point{steady - epoch}; + } +}; + +const std::chrono::steady_clock::time_point recent_epoch_float_clock::epoch = + std::chrono::steady_clock::now(); + +void +test_wait_until() +{ + using clock = recent_epoch_float_clock; + + std::mutex mx; + std::condition_variable cv; + + for (int i = 0; i < 3; ++i) + { + std::unique_lock<std::mutex> l(mx); + const auto start = clock::now(); + const auto wait_time = start + std::chrono::duration<float>{1.0}; + + // In theory we could get a spurious wakeup, but in practice we won't. + const auto result = cv.wait_until(l, wait_time); + + VERIFY( result == std::cv_status::timeout ); + const auto elapsed = clock::now() - start; + VERIFY( elapsed >= std::chrono::seconds(1) ); + } +} + int main() { - test01(); + test_wait_for(); + test_wait_until(); }
reply other threads:[~2020-09-11 13:29 UTC|newest] Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200911132943.3F4B1398750C@sourceware.org \ --to=redi@gcc.gnu.org \ --cc=gcc-cvs@gcc.gnu.org \ --cc=libstdc++-cvs@gcc.gnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).