-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-88494: Reduce CLOCK_RES to 1 ms in tests #118395
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
Conversation
On Windows, time.monotonic() now calls QueryPerformanceCounter() which has a resolution of 100 ns. Reduce CLOCK_RES from 50 or 100 ms to 1 ms.
Sadly, my change is too optimistic :-( test_asyncio failed on Windows:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I worry that other tests (that still use the 1ms value) will become flaky on Windows, unless you have a clear explanation why that fix is only needed in that one spot.
asyncio.BaseEventLoop.time() is time.monotonic(). On Windows, this clock now has a resolution of 100 ns, since I modified it to use QueryPerformanceCounter().
This test calls RegisterWaitForSingleObject() which has a resolution of 15.6 ms. I scheduled buildbot jobs to see how the change goes on more various machines. |
Failure on AMD64 Windows Server 2022 NoGIL PR:
|
Maybe the problem is that the meaning of the variable CLOCK_RES is ambiguous? It looks like it gives the actual clock resolution, but it actually seems to mean the tolerance for longer (or shorter) delays. Tests depending on close-to-exact delays will always be flaky -- or if we make the tolerance too large, they will be meaningless. |
While it's appealing to make the test stricter, CLOCK_RES is used to compare time.monotonic() values (resolution better than 1 us), but also to compare Windows "wait" functions (resolution of 15.6 ms). I prefer to abandon this PR. I don't want to have multiple CLOCK_RES depending on the function and/or the operating system. Let's keep the status quo of 50-100 ms tolerance. |
Good call. Sorry it didn't work out, but they can't all be winners... :-) |
On Windows, time.monotonic() now calls QueryPerformanceCounter() which has a resolution of 100 ns. Reduce CLOCK_RES from 50 or 100 ms to 1 ms.