Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make this_thread::sleep_until clock-aware #3914

Merged
merged 10 commits into from
Oct 20, 2023

Conversation

achabense
Copy link
Contributor

@achabense achabense commented Jul 31, 2023

Partially addresses #718.

Fixes this_thread::sleep_until / sleep_for:

  • replace _Thrd_sleep with _Thrd_sleep_for, which is a direct call to windows Sleep (_Thrd_sleep also relies on Sleep, but is bound to system-clock).
  • replace usage of _To_timespec64_sys_10_day_clamped with clock-aware logic in sleep_until.

Thanks to @AlexGuteniev for helping me figure out how to add import-library functions ❤️

@achabense achabense requested a review from a team as a code owner July 31, 2023 17:10
stl/src/sharedmutex.cpp Outdated Show resolved Hide resolved
stl/src/sharedmutex.cpp Outdated Show resolved Hide resolved
stl/inc/xthreads.h Outdated Show resolved Hide resolved
@AlexGuteniev
Copy link
Contributor

There are still the following functions:

_CRTIMP2_PURE int __cdecl _Mtx_timedlock(_Mtx_t, const xtime*);
_CRTIMP2_PURE int __cdecl _Cnd_timedwait(_Cnd_t, _Mtx_t, const xtime*);

I think we should either not resolve the original issue or create a follow-up issue.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jul 31, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jul 31, 2023
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving these comments in my capacity as an individual; I'm no longer a maintainer and won't be following this PR going forward, but I think the folks who actually are maintainers should consider them. In particular, I feel strongly that ns is not the correct type when all platform APIs speak FILETIME 100ns units. (The other comments are really nitpicks)

stl/inc/xthreads.h Outdated Show resolved Hide resolved
stl/inc/thread Outdated Show resolved Hide resolved
stl/src/sharedmutex.cpp Outdated Show resolved Hide resolved
@achabense
Copy link
Contributor Author

achabense commented Aug 1, 2023

In this pr, should I:

  1. make _Thrd_sleep_for accept 100ns instead?
  2. make _Reltime_ns_10_day_clamped more general?
    2.1. let this function take _Now and _Then as parameters, so that not to call _Clock::now() within the function? (Also have to decide whether to take _Now<(=)_Then as precondition)
    2.2. make clamp value a template parameter?
    2.3. make return type a template parameter?

I think these can be quality enhancements in the future:

  1. Adopt more accurate machinery for _Thrd_sleep_for.
  2. In wait_until, make sleep intervals more fine-grained if _Clock is not steady.

@achabense
Copy link
Contributor Author

achabense commented Aug 3, 2023

Though a bit off-topic, I think it's not harmful to make a push to add if constexpr in these places in this pr:

constexpr bool _Num_is_one = _CF::num == 1;
constexpr bool _Den_is_one = _CF::den == 1;
if (_Den_is_one) {
if (_Num_is_one) {
return static_cast<_To>(static_cast<_ToRep>(_Dur.count()));
} else {
return static_cast<_To>(
static_cast<_ToRep>(static_cast<_CR>(_Dur.count()) * static_cast<_CR>(_CF::num)));
}
} else {
if (_Num_is_one) {

@frederick-vs-ja

This comment was marked as resolved.

@achabense

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

In my opinion, the separately compiled code should generally be a thin wrapper over the Windows API. Except when we need modifiable global state, the purpose of the DLL/LIB is to avoid dragging in <Windows.h> - we prefer to have as much code as possible be header-only. (Older code didn't adhere to this discipline which is where a lot of our problems come from.)

@AlexGuteniev
Copy link
Contributor

we prefer to have as much code as possible be header-only

<charconv> tables seem to disagree.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 3, 2023

I originally implemented them header-only! 😸 (And while there are throughput advantages to separately compiling them, there are also costs - we will never be able to remove them even after switching algorithms.) Basically there are always reasons for special exceptions, but that's the general rule - code in headers is more flexible and less burdensome for maintenance.

@S0SLA
Copy link

S0SLA commented Aug 6, 2023

By achabense:

I’m sorry that I cannot take part in development these days. Before this I’ve made a feedback branch to 1. make _Thrd_sleep_for a direct call to Sleep. 2. merge _Reltime… template into sleep_for, as I think its too early to generalize this utility. Please help mark this pr work in progress temporarily, I will go back as soon as I have time(maybe several days later).
As to the constexpr if, I think it would be nice to make an issue for some newcomers to do the contribution.

@achabense

This comment was marked as outdated.

@StephanTLavavej
Copy link
Member

Yes, after a brief glance the direction of that branch looks good to us; by the way, we now use __stdcall in new code because it's slightly more efficient (smaller code size since the callee pops instead of the caller), we should follow this convention even though it means that the declaration will differ compared to the nearby shared_mutex support functions.

@achabense

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@achabense
Copy link
Contributor Author

There are still the following functions:

_CRTIMP2_PURE int __cdecl _Mtx_timedlock(_Mtx_t, const xtime*);
_CRTIMP2_PURE int __cdecl _Cnd_timedwait(_Cnd_t, _Mtx_t, const xtime*);

I think we should either not resolve the original issue or create a follow-up issue.

I've changed my pr description to avoid closing the original issue.

@StephanTLavavej StephanTLavavej self-assigned this Aug 16, 2023
stl/inc/xthreads.h Outdated Show resolved Hide resolved
@achabense
Copy link
Contributor Author

Can we also add back the piece of test mentioned in #1642?

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll push changes for these nearly non-functional comments.

stl/src/sharedmutex.cpp Outdated Show resolved Hide resolved
stl/inc/thread Show resolved Hide resolved
stl/inc/xthreads.h Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Oct 18, 2023
@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 19ec9c1 into microsoft:main Oct 20, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

🧵 💤 ⏰

@Mike4Online
Copy link

Incidentally, this change appears to be a breaking change. I recently built the azure-sdk-cpp and aws-sdk-cpp libraries (for Azure Blob Storage and AWS S3, using vckpg port names azure-storage-blobs-cpp and aws-sdk-cpp[core,s3,s3-crt], respectively) using vcpkg on a Windows 10 system running Visual Studio 2022 Build 17.9. I then tried to build a DLL that links with these (now pre-built) static libraries on a different PC that was running Visual Studio 2022 Build 17.8. I got linker errors:

    49>azure-storage-blobs.lib(blob_responses.cpp.obj) : error LNK2019: unresolved external symbol __Thrd_sleep_for@4 referenced in function "void __cdecl std::this_thread::sleep_until<struct std::chrono::steady_clock,class std::chrono::duration<__int64,struct std::ratio<1,1000000000> > >(class std::chrono::time_point<struct std::chrono::steady_clock,class std::chrono::duration<__int64,struct std::ratio<1,1000000000> > > const &)" (??$sleep_until@Usteady_clock@chrono@std@@V?$duration@_JU?$ratio@$00$0DLJKMKAA@@std@@@23@@this_thread@std@@YAXABV?$time_point@Usteady_clock@chrono@std@@V?$duration@_JU?$ratio@$00$0DLJKMKAA@@std@@@23@@chrono@1@@Z) [D:\work\DevJenkins\Windows\MetaDataCacheLib\MetaDataCacheDll\MetaDataCacheDll.vcxproj]
    49>azure-core.lib(retry_policy.cpp.obj) : error LNK2001: unresolved external symbol __Thrd_sleep_for@4 [D:\work\DevJenkins\Windows\MetaDataCacheLib\MetaDataCacheDll\MetaDataCacheDll.vcxproj]
    49>aws-cpp-sdk-core.lib(ub_core.cpp.obj) : error LNK2001: unresolved external symbol __Thrd_sleep_for@4 [D:\work\DevJenkins\Windows\MetaDataCacheLib\MetaDataCacheDll\MetaDataCacheDll.vcxproj]

As soon as I upgraded my other PC to Visual Studio 2022 build 17.9 the link succeeded.

FYI.

@AlexGuteniev
Copy link
Contributor

using vcpkg on a Windows 10 system running Visual Studio 2022 Build 17.9. I then tried to build a DLL that links with these (now pre-built) static libraries on a different PC that was running Visual Studio 2022 Build 17.8

This is expected.
Mixing multiple ABI compatible versions is supported, but the STL binaries should always be from the latest version in this case.

@StephanTLavavej
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants