From d62f9140651c67600d474765730497a7b91a1c99 Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sun, 30 Jun 2024 15:28:45 +0300 Subject: [PATCH 1/4] Wait `condition_variable` for `steady_clock` --- stl/inc/condition_variable | 42 ++++++++++++++--------------- stl/inc/mutex | 2 +- stl/inc/xthreads.h | 3 ++- stl/src/sharedmutex.cpp | 45 +++++++++++++++++++++---------- tests/libcxx/expected_results.txt | 8 ------ 5 files changed, 54 insertions(+), 46 deletions(-) diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable index c72cabc8cb..095eef9234 100644 --- a/stl/inc/condition_variable +++ b/stl/inc/condition_variable @@ -123,12 +123,26 @@ public: } const auto _Rel_time_ms = _Clamped_rel_time_ms_count(_Rel_time); - const cv_status _Result = _Wait_for_ms_count(_Lck, _Rel_time_ms._Count); - if (_Rel_time_ms._Clamped) { - return cv_status::no_timeout; - } - return _Result; + // wait for signal with timeout + const shared_ptr _Ptr = _Myptr; // for immunity to *this destruction + const auto _Start = _CHRONO steady_clock::now(); + const auto _Target = _Start + _Rel_time; + + unique_lock _Guard{*_Ptr}; + _Unlock_guard<_Lock> _Unlock_outer{_Lck}; + const _Thrd_result _Res = _Cnd_timedwait_for_unchecked(_Mycnd(), _Ptr->_Mymtx(), _Rel_time_ms._Count); + _Guard.unlock(); + + if (_Res == _Thrd_result::_Success) { + return cv_status::no_timeout; // Real or WinAPI-internal spurious wake + } else if (_Rel_time_ms._Clamped) { + return cv_status::no_timeout; // Spuriously wake due to clamped timeout + } else { + const auto _Now = std::chrono::steady_clock::now(); + // Spuriously wake if timed out prematurely + return (_Now >= _Target) ? cv_status::timeout : cv_status::no_timeout; + } } template @@ -202,7 +216,7 @@ public: } const unsigned long _Rel_ms_count = _Clamped_rel_time_ms_count(_Abs_time - _Now)._Count; - (void) _Cnd_timedwait_for(_Mycnd(), _Myptr->_Mymtx(), _Rel_ms_count); + (void) _Cnd_timedwait_for_unchecked(_Mycnd(), _Myptr->_Mymtx(), _Rel_ms_count); _Guard_unlocks_before_locking_outer.unlock(); } // relock @@ -223,22 +237,6 @@ private: _NODISCARD _Cnd_t _Mycnd() noexcept { return &_Cnd_storage; } - - template - cv_status _Wait_for_ms_count(_Lock& _Lck, const unsigned int _Rel_ms_count) { - // wait for signal with timeout - const shared_ptr _Ptr = _Myptr; // for immunity to *this destruction - unique_lock _Guard{*_Ptr}; - _Unlock_guard<_Lock> _Unlock_outer{_Lck}; - const _Thrd_result _Res = _Cnd_timedwait_for(_Mycnd(), _Ptr->_Mymtx(), _Rel_ms_count); - _Guard.unlock(); - - if (_Res == _Thrd_result::_Success) { - return cv_status::no_timeout; - } else { - return cv_status::timeout; - } - } }; _EXPORT_STD inline void notify_all_at_thread_exit(condition_variable& _Cnd, unique_lock _Lck) noexcept diff --git a/stl/inc/mutex b/stl/inc/mutex index 3bf8b71ce6..0daa8314b5 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -599,7 +599,7 @@ public: const unsigned long _Rel_ms_count = _Clamped_rel_time_ms_count(_Abs_time - _Now)._Count; - const _Thrd_result _Res = _Cnd_timedwait_for(_Mycnd(), _Lck.mutex()->_Mymtx(), _Rel_ms_count); + const _Thrd_result _Res = _Cnd_timedwait_for_unchecked(_Mycnd(), _Lck.mutex()->_Mymtx(), _Rel_ms_count); if (_Res == _Thrd_result::_Success) { return cv_status::no_timeout; } diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index 09dad03f55..de50b11a5e 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -62,7 +62,8 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_signal(_Cnd_t) noexcept; // TRANSITION, _CRTIMP2_PURE void __cdecl _Cnd_register_at_thread_exit(_Cnd_t, _Mtx_t, int*) noexcept; _CRTIMP2_PURE void __cdecl _Cnd_unregister_at_thread_exit(_Mtx_t) noexcept; _CRTIMP2_PURE void __cdecl _Cnd_do_broadcast_at_thread_exit() noexcept; -_Thrd_result __stdcall _Cnd_timedwait_for(_Cnd_t, _Mtx_t, unsigned int) noexcept; +// '_unchecked' means it is not checked against the 'steady_clock', so may report timeout prematurely +_Thrd_result __stdcall _Cnd_timedwait_for_unchecked(_Cnd_t, _Mtx_t, unsigned int) noexcept; } // extern "C" _STD_BEGIN diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index ecb73c754c..c3ea1b7ebf 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -41,25 +41,42 @@ void __stdcall _Thrd_sleep_for(const unsigned long ms) noexcept { // suspend cur Sleep(ms); } -_Thrd_result __stdcall _Cnd_timedwait_for(const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms) noexcept { - _Thrd_result res = _Thrd_result::_Success; - const auto start_ms = GetTickCount64(); +namespace { + _Thrd_result __stdcall _Cnd_timedwait_for_impl( + const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms, bool checked) noexcept { + _Thrd_result res = _Thrd_result::_Success; + unsigned long long start_ms = 0; + + if (checked) { + start_ms = GetTickCount64(); + } - // TRANSITION: replace with _Mtx_clear_owner(mtx); - mtx->_Thread_id = -1; - --mtx->_Count; + // TRANSITION: replace with _Mtx_clear_owner(mtx); + mtx->_Thread_id = -1; + --mtx->_Count; - if (!_Primitive_wait_for(cond, mtx, target_ms)) { // report timeout - const auto end_ms = GetTickCount64(); - if (end_ms - start_ms >= target_ms) { - res = _Thrd_result::_Timedout; + if (!_Primitive_wait_for(cond, mtx, target_ms)) { // report timeout + if (!checked || GetTickCount64() - start_ms >= target_ms) { + res = _Thrd_result::_Timedout; + } } + // TRANSITION: replace with _Mtx_reset_owner(mtx); + mtx->_Thread_id = static_cast(GetCurrentThreadId()); + ++mtx->_Count; + + return res; } - // TRANSITION: replace with _Mtx_reset_owner(mtx); - mtx->_Thread_id = static_cast(GetCurrentThreadId()); - ++mtx->_Count; +} // namespace - return res; +// TRANSITION, ABI: preserved for compatibility +_Thrd_result __stdcall _Cnd_timedwait_for(const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms) noexcept { + return _Cnd_timedwait_for_impl(cond, mtx, target_ms, true); } +_Thrd_result __stdcall _Cnd_timedwait_for_unchecked( + const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms) noexcept { + return _Cnd_timedwait_for_impl(cond, mtx, target_ms, false); +} + + } // extern "C" diff --git a/tests/libcxx/expected_results.txt b/tests/libcxx/expected_results.txt index d7b1459195..c0ffa51961 100644 --- a/tests/libcxx/expected_results.txt +++ b/tests/libcxx/expected_results.txt @@ -453,14 +453,6 @@ std/input.output/string.streams/stringbuf/stringbuf.members/view.pass.cpp FAIL std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/dtor.pass.cpp FAIL std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/emit.pass.cpp FAIL -# GH-4723: : condition_variable_any::wait_for returns cv_status::timeout when the elapsed time is shorter than requested -std/thread/thread.condition/thread.condition.condvarany/notify_all.pass.cpp SKIPPED -std/thread/thread.condition/thread.condition.condvarany/notify_one.pass.cpp SKIPPED -std/thread/thread.condition/thread.condition.condvarany/wait_for_pred.pass.cpp SKIPPED -std/thread/thread.condition/thread.condition.condvarany/wait_for.pass.cpp SKIPPED -std/thread/thread.condition/thread.condition.condvarany/wait_until_pred.pass.cpp SKIPPED -std/thread/thread.condition/thread.condition.condvarany/wait_until.pass.cpp SKIPPED - # *** VCRUNTIME BUGS *** # DevCom-10373274 VSO-1824997 "vcruntime nothrow array operator new falls back on the wrong function" From 3895ff502cb9467851217390442f62bfb15989ec Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 2 Aug 2024 01:44:50 -0700 Subject: [PATCH 2/4] Use `_CHRONO`. --- stl/inc/condition_variable | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable index 095eef9234..536997eda6 100644 --- a/stl/inc/condition_variable +++ b/stl/inc/condition_variable @@ -139,7 +139,7 @@ public: } else if (_Rel_time_ms._Clamped) { return cv_status::no_timeout; // Spuriously wake due to clamped timeout } else { - const auto _Now = std::chrono::steady_clock::now(); + const auto _Now = _CHRONO steady_clock::now(); // Spuriously wake if timed out prematurely return (_Now >= _Target) ? cv_status::timeout : cv_status::no_timeout; } From 9c2fec2bc6ad5676c37b2f14048df0a7290ccce0 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 2 Aug 2024 01:46:46 -0700 Subject: [PATCH 3/4] Add const. --- stl/src/sharedmutex.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index c3ea1b7ebf..fac73f1e38 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -43,7 +43,7 @@ void __stdcall _Thrd_sleep_for(const unsigned long ms) noexcept { // suspend cur namespace { _Thrd_result __stdcall _Cnd_timedwait_for_impl( - const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms, bool checked) noexcept { + const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms, const bool checked) noexcept { _Thrd_result res = _Thrd_result::_Success; unsigned long long start_ms = 0; From 541b546d9bbd62d59bbb4d4236d18a48e5f7b114 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 2 Aug 2024 02:05:26 -0700 Subject: [PATCH 4/4] Continue `else if` chain for spurious wakes. --- stl/inc/condition_variable | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable index 536997eda6..2931b1b03f 100644 --- a/stl/inc/condition_variable +++ b/stl/inc/condition_variable @@ -138,10 +138,10 @@ public: return cv_status::no_timeout; // Real or WinAPI-internal spurious wake } else if (_Rel_time_ms._Clamped) { return cv_status::no_timeout; // Spuriously wake due to clamped timeout + } else if (_CHRONO steady_clock::now() < _Target) { + return cv_status::no_timeout; // Spuriously wake due to premature timeout } else { - const auto _Now = _CHRONO steady_clock::now(); - // Spuriously wake if timed out prematurely - return (_Now >= _Target) ? cv_status::timeout : cv_status::no_timeout; + return cv_status::timeout; } }