From 919c787a359fc162ea2f9e7d2419d21286cef81f Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Mon, 31 Jul 2023 03:41:58 +0800 Subject: [PATCH 1/7] fix --- stl/inc/thread | 28 +++++++++++++++++++++------- stl/inc/xthreads.h | 2 +- stl/src/cthread.cpp | 1 + stl/src/sharedmutex.cpp | 5 +++++ 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/stl/inc/thread b/stl/inc/thread index 17d0e8509d2..248c09e89f7 100644 --- a/stl/inc/thread +++ b/stl/inc/thread @@ -173,6 +173,23 @@ _NODISCARD auto _To_absolute_time(const chrono::duration<_Rep, _Period>& _Rel_ti return _Abs_time; } +template +_NODISCARD chrono::nanoseconds _Reltime_ns_10_day_clamped(const chrono::time_point<_Clock, _Duration>& _Then) { + // returns _Then - _Now, clamped to [0, ten days] + const auto _Now = _Clock::now(); + if (_Now >= _Then) { + return chrono::nanoseconds::zero(); + } else { + constexpr chrono::nanoseconds _Ten_days{chrono::hours{24} * 10}; + const auto _Rel = _Then - _Now; + if (_Rel >= _Ten_days) { + return _Ten_days; + } else { + return chrono::duration_cast(_Rel); + } + } +} + namespace this_thread { _EXPORT_STD _NODISCARD thread::id get_id() noexcept; @@ -186,14 +203,11 @@ namespace this_thread { static_assert(chrono::is_clock_v<_Clock>, "Clock type required"); #endif // _HAS_CXX20 for (;;) { - const auto _Now = _Clock::now(); - if (_Abs_time <= _Now) { - return; + const long long _Rel_ns = _Reltime_ns_10_day_clamped(_Abs_time).count(); + if (_Rel_ns <= 0) { + break; } - - _timespec64 _Tgt; - (void) _To_timespec64_sys_10_day_clamped(_Tgt, _Abs_time - _Now); - _Thrd_sleep(&_Tgt); + _Thrd_sleep_for(_Rel_ns); } } diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index e2bdf732c06..ba98143b2c3 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -91,7 +91,6 @@ enum class _Thrd_result : int { _Success, _Nomem, _Timedout, _Busy, _Error }; // threads _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_detach(_Thrd_t); _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_join(_Thrd_t, int*); -_CRTIMP2_PURE void __cdecl _Thrd_sleep(const _timespec64*); _CRTIMP2_PURE void __cdecl _Thrd_yield(); _CRTIMP2_PURE unsigned int __cdecl _Thrd_hardware_concurrency(); _CRTIMP2_PURE _Thrd_id_t __cdecl _Thrd_id(); @@ -126,6 +125,7 @@ int __cdecl _Smtx_try_lock_exclusive(_Smtx_t*); int __cdecl _Smtx_try_lock_shared(_Smtx_t*); void __cdecl _Smtx_unlock_exclusive(_Smtx_t*); void __cdecl _Smtx_unlock_shared(_Smtx_t*); +void __cdecl _Thrd_sleep_for(long long /*ns*/); // TRANSITION: defined in sharedmutex.cpp for convenience // condition variables _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t*); diff --git a/stl/src/cthread.cpp b/stl/src/cthread.cpp index 389b8ee9cda..cc1d04a3bd6 100644 --- a/stl/src/cthread.cpp +++ b/stl/src/cthread.cpp @@ -72,6 +72,7 @@ _Thrd_result _Thrd_detach(_Thrd_t thr) { // tell OS to release thread's resource return CloseHandle(thr._Hnd) ? _Thrd_result::_Success : _Thrd_result::_Error; } +// TRANSITION, ABI: _Thrd_sleep() is preserved for binary compatibility void _Thrd_sleep(const _timespec64* xt) { // suspend thread until time xt _timespec64 now; _Timespec64_get_sys(&now); diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index 6cfcc63cf45..5bf02794369 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -36,4 +36,9 @@ void __cdecl _Smtx_unlock_exclusive(_Smtx_t* smtx) { // unlock exclusive shared void __cdecl _Smtx_unlock_shared(_Smtx_t* smtx) { // unlock non-exclusive shared mutex ReleaseSRWLockShared(reinterpret_cast(smtx)); } + +void __cdecl _Thrd_sleep_for(const long long ns) { // attempt to suspend thread for at least `ns` nanoseconds + constexpr long _Nsec_per_msec = 1'000'000L; + Sleep(static_cast((ns + _Nsec_per_msec - 1) / _Nsec_per_msec)); +} } From 801fda7f6a14c6e09fbedfb965d657209e0e9774 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Tue, 1 Aug 2023 01:19:03 +0800 Subject: [PATCH 2/7] break->return --- stl/inc/thread | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/thread b/stl/inc/thread index 248c09e89f7..1ba31290f5e 100644 --- a/stl/inc/thread +++ b/stl/inc/thread @@ -205,7 +205,7 @@ namespace this_thread { for (;;) { const long long _Rel_ns = _Reltime_ns_10_day_clamped(_Abs_time).count(); if (_Rel_ns <= 0) { - break; + return; } _Thrd_sleep_for(_Rel_ns); } From 0020d4ff14ccafa4d0c7393837a4ebd6f88a7e46 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Tue, 1 Aug 2023 01:56:45 +0800 Subject: [PATCH 3/7] review feedack: _Nsec_per_msec->nsec_per_msec --- stl/src/sharedmutex.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index 5bf02794369..e96865af795 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -38,7 +38,7 @@ void __cdecl _Smtx_unlock_shared(_Smtx_t* smtx) { // unlock non-exclusive shared } void __cdecl _Thrd_sleep_for(const long long ns) { // attempt to suspend thread for at least `ns` nanoseconds - constexpr long _Nsec_per_msec = 1'000'000L; - Sleep(static_cast((ns + _Nsec_per_msec - 1) / _Nsec_per_msec)); + constexpr long nsec_per_msec = 1'000'000L; + Sleep(static_cast((ns + nsec_per_msec - 1) / nsec_per_msec)); } } From 79cf55a338b2bce085571c9dd28441fc4e54505f Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Thu, 10 Aug 2023 09:29:08 +0800 Subject: [PATCH 4/7] apply feedback branch --- stl/inc/thread | 30 ++++++++++-------------------- stl/inc/xthreads.h | 2 +- stl/src/sharedmutex.cpp | 5 ++--- 3 files changed, 13 insertions(+), 24 deletions(-) diff --git a/stl/inc/thread b/stl/inc/thread index 1ba31290f5e..ba096e1073c 100644 --- a/stl/inc/thread +++ b/stl/inc/thread @@ -173,23 +173,6 @@ _NODISCARD auto _To_absolute_time(const chrono::duration<_Rep, _Period>& _Rel_ti return _Abs_time; } -template -_NODISCARD chrono::nanoseconds _Reltime_ns_10_day_clamped(const chrono::time_point<_Clock, _Duration>& _Then) { - // returns _Then - _Now, clamped to [0, ten days] - const auto _Now = _Clock::now(); - if (_Now >= _Then) { - return chrono::nanoseconds::zero(); - } else { - constexpr chrono::nanoseconds _Ten_days{chrono::hours{24} * 10}; - const auto _Rel = _Then - _Now; - if (_Rel >= _Ten_days) { - return _Ten_days; - } else { - return chrono::duration_cast(_Rel); - } - } -} - namespace this_thread { _EXPORT_STD _NODISCARD thread::id get_id() noexcept; @@ -203,11 +186,18 @@ namespace this_thread { static_assert(chrono::is_clock_v<_Clock>, "Clock type required"); #endif // _HAS_CXX20 for (;;) { - const long long _Rel_ns = _Reltime_ns_10_day_clamped(_Abs_time).count(); - if (_Rel_ns <= 0) { + const auto _Now = _Clock::now(); + if (_Now >= _Abs_time) { return; } - _Thrd_sleep_for(_Rel_ns); + constexpr chrono::milliseconds _Ten_days{chrono::hours{24} * 10}; + const auto _Rel = _Abs_time - _Now; + if (_Rel >= _Ten_days) { + _Thrd_sleep_for(static_cast(_Ten_days.count())); + } else { + const auto _Rel_ms = chrono::ceil(_Rel); + _Thrd_sleep_for(static_cast(_Rel_ms.count())); + } } } diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index ba98143b2c3..69366d9cf4a 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -125,7 +125,7 @@ int __cdecl _Smtx_try_lock_exclusive(_Smtx_t*); int __cdecl _Smtx_try_lock_shared(_Smtx_t*); void __cdecl _Smtx_unlock_exclusive(_Smtx_t*); void __cdecl _Smtx_unlock_shared(_Smtx_t*); -void __cdecl _Thrd_sleep_for(long long /*ns*/); // TRANSITION: defined in sharedmutex.cpp for convenience +void __stdcall _Thrd_sleep_for(unsigned long /*ms*/); // TRANSITION: defined in sharedmutex.cpp for convenience // condition variables _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t*); diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index e96865af795..8c70bc8587c 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -37,8 +37,7 @@ void __cdecl _Smtx_unlock_shared(_Smtx_t* smtx) { // unlock non-exclusive shared ReleaseSRWLockShared(reinterpret_cast(smtx)); } -void __cdecl _Thrd_sleep_for(const long long ns) { // attempt to suspend thread for at least `ns` nanoseconds - constexpr long nsec_per_msec = 1'000'000L; - Sleep(static_cast((ns + nsec_per_msec - 1) / nsec_per_msec)); +void __stdcall _Thrd_sleep_for(const unsigned long ms) { // suspend current thread + Sleep(ms); } } From 9a47f8ac240e4579a1b9664cb9e2cf30ec2b483f Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Thu, 17 Aug 2023 00:29:45 +0800 Subject: [PATCH 5/7] style --- stl/inc/thread | 8 ++++---- stl/inc/xthreads.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/stl/inc/thread b/stl/inc/thread index ba096e1073c..cfcbc67dd19 100644 --- a/stl/inc/thread +++ b/stl/inc/thread @@ -187,13 +187,13 @@ namespace this_thread { #endif // _HAS_CXX20 for (;;) { const auto _Now = _Clock::now(); - if (_Now >= _Abs_time) { + if (_Abs_time <= _Now) { return; } - constexpr chrono::milliseconds _Ten_days{chrono::hours{24} * 10}; + constexpr chrono::milliseconds _Clamp{chrono::hours{24}}; const auto _Rel = _Abs_time - _Now; - if (_Rel >= _Ten_days) { - _Thrd_sleep_for(static_cast(_Ten_days.count())); + if (_Rel >= _Clamp) { + _Thrd_sleep_for(static_cast(_Clamp.count())); } else { const auto _Rel_ms = chrono::ceil(_Rel); _Thrd_sleep_for(static_cast(_Rel_ms.count())); diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index 69366d9cf4a..7a4d05d5cc5 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -125,7 +125,7 @@ int __cdecl _Smtx_try_lock_exclusive(_Smtx_t*); int __cdecl _Smtx_try_lock_shared(_Smtx_t*); void __cdecl _Smtx_unlock_exclusive(_Smtx_t*); void __cdecl _Smtx_unlock_shared(_Smtx_t*); -void __stdcall _Thrd_sleep_for(unsigned long /*ms*/); // TRANSITION: defined in sharedmutex.cpp for convenience +void __stdcall _Thrd_sleep_for(unsigned long /*ms*/); // defined in sharedmutex.cpp for convenience // condition variables _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t*); From 09ab20b2bd7b0c5d5a1e36b02683ac05dfb787c1 Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Thu, 17 Aug 2023 21:34:15 +0800 Subject: [PATCH 6/7] style --- stl/inc/xthreads.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index 5005beab55a..bd5c53d9955 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -86,6 +86,7 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_join(_Thrd_t, int*); _CRTIMP2_PURE void __cdecl _Thrd_yield(); _CRTIMP2_PURE unsigned int __cdecl _Thrd_hardware_concurrency(); _CRTIMP2_PURE _Thrd_id_t __cdecl _Thrd_id(); +void __stdcall _Thrd_sleep_for(unsigned long /*ms*/); // defined in sharedmutex.cpp for convenience // mutexes enum { // mutex types @@ -119,7 +120,6 @@ int __cdecl _Smtx_try_lock_exclusive(_Smtx_t*); int __cdecl _Smtx_try_lock_shared(_Smtx_t*); void __cdecl _Smtx_unlock_exclusive(_Smtx_t*); void __cdecl _Smtx_unlock_shared(_Smtx_t*); -void __stdcall _Thrd_sleep_for(unsigned long /*ms*/); // defined in sharedmutex.cpp for convenience // condition variables #ifdef _CRTBLD From 489cd7dc1ee1869d3b8f8594ea4ffdf235028c01 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Wed, 18 Oct 2023 10:55:23 -0700 Subject: [PATCH 7/7] Casey's review comments --- stl/inc/thread | 3 +++ stl/inc/xthreads.h | 2 +- stl/src/sharedmutex.cpp | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/stl/inc/thread b/stl/inc/thread index 56c1de52962..83b2c624737 100644 --- a/stl/inc/thread +++ b/stl/inc/thread @@ -196,7 +196,10 @@ namespace this_thread { if (_Abs_time <= _Now) { return; } + + // _Clamp must be less than 2^32 - 1 (INFINITE) milliseconds, but is otherwise arbitrary. constexpr chrono::milliseconds _Clamp{chrono::hours{24}}; + const auto _Rel = _Abs_time - _Now; if (_Rel >= _Clamp) { _Thrd_sleep_for(static_cast(_Clamp.count())); diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index bd5c53d9955..6ebc4e90f1d 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -86,7 +86,7 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_join(_Thrd_t, int*); _CRTIMP2_PURE void __cdecl _Thrd_yield(); _CRTIMP2_PURE unsigned int __cdecl _Thrd_hardware_concurrency(); _CRTIMP2_PURE _Thrd_id_t __cdecl _Thrd_id(); -void __stdcall _Thrd_sleep_for(unsigned long /*ms*/); // defined in sharedmutex.cpp for convenience +void __stdcall _Thrd_sleep_for(unsigned long /*ms*/) noexcept; // mutexes enum { // mutex types diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index 8c70bc8587c..c013e7776fa 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -37,7 +37,7 @@ void __cdecl _Smtx_unlock_shared(_Smtx_t* smtx) { // unlock non-exclusive shared ReleaseSRWLockShared(reinterpret_cast(smtx)); } -void __stdcall _Thrd_sleep_for(const unsigned long ms) { // suspend current thread +void __stdcall _Thrd_sleep_for(const unsigned long ms) noexcept { // suspend current thread for `ms` milliseconds Sleep(ms); } }