[Threading] Always round up when converting std::chrono::durations. #61352

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.

Sign up for GitHub

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

Jump to bottom

Merged

al45tair merged 4 commits into swiftlang:main from al45tair:eng/PR-100236038-2

Sep 30, 2022

Contributor

al45tair commented Sep 29, 2022

We were inadvertently rounding down, which was fine almost 100% of the time, but on Windows where the timer resolution is only 1ms, it made the test very slightly flaky because we were asking to wait for 9ms rather than 10ms, then checking we'd waited at least 10ms.

We should explicitly round up, for that reason.

rdar://100236038


          [Threading] Always round up when converting std::chrono::durations.

fcf3bfa

We were inadvertently rounding down, which was fine almost 100% of the
time, but on Windows where the timer resolution is only 1ms, it made
the test _very slightly_ flaky because we were asking to wait for
9ms rather than 10ms, then checking we'd waited at least 10ms.

We should explicitly round up, for that reason.

rdar://100236038

Contributor Author

al45tair commented Sep 29, 2022

@swift-ci Please test

al45tair requested review from mikeash and compnerd

September 29, 2022 09:54

Contributor Author

al45tair commented Sep 29, 2022

Hmmm. Seems this wasn't the only problem on Windows making the test flaky. I wonder if the server applied a time adjustment during the SleepConditionVariableSRW() call. Maybe the thing to do is to change the test to allow things to return 10% "sooner" than it thinks they should?

Contributor Author

al45tair commented Sep 29, 2022

No, I don't think the time adjustment explanation is the right one. That shouldn't really be happening because we're using steady_clock. I think it might be the odd behaviour of Sleep functions on Windows, where (apparently) if you try to wait for less than a system tick (by default 1/64s), they can return early.

Let's try increasing the system tick rate during the test. I'll run the tests a few times to make sure it works.

Contributor Author

al45tair commented Sep 29, 2022

@swift-ci Please test Windows platform

al45tair force-pushed the eng/PR-100236038-2 branch from c008d75 to a9f0758 Compare

September 29, 2022 13:09

mikeash approved these changes

View reviewed changes

Contributor

mikeash left a comment

LGTM, once it works.

Contributor Author

al45tair commented Sep 29, 2022

@swift-ci Please test Windows platform

al45tair force-pushed the eng/PR-100236038-2 branch from a9f0758 to fe639b5 Compare

September 29, 2022 14:26

Contributor Author

al45tair commented Sep 29, 2022

@swift-ci Please test Windows platform

compnerd approved these changes

View reviewed changes

include/swift/Threading/Impl/chrono.h Outdated Show resolved Hide resolved


          [UnitTests][Threading] Tweak ConditionVariable test for Windows.

10ab608

Windows' behaviour wrt `Sleep` family functions can be odd.  Apparently,
if you specify a time lower than a system tick, they can return early.
This makes the test flaky when it shouldn't be.  To fix it, we use the
Multimedia functions to adjust the system tick count as low as we can
get it.

rdar://100236038

al45tair force-pushed the eng/PR-100236038-2 branch from fe639b5 to 10ab608 Compare

September 29, 2022 14:41


          [Threading] Rename chrono.h to match the namespace name.

33e74cd

rdar://100236038

Contributor Author

al45tair commented Sep 29, 2022

@swift-ci Please test Windows platform


          [Threading][Win32] Fix the wait functions for Win32 to always over-wait.

d0f42ca

Rather than bodging the test to make it more robust, fix the functions
in the Threading layer to behave the same as they do on other platforms,
i.e. to guarantee that they always wait *at least* the amount of time
you asked for.

rdar://100236038

Contributor Author

al45tair commented Sep 29, 2022

@swift-ci Please test Windows platform

Contributor Author

al45tair commented Sep 29, 2022

Woo! That seems to have worked.

Contributor Author

al45tair commented Sep 29, 2022

@swift-ci please smoke test

al45tair merged commit 7d840f1 into swiftlang:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet