Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
On Windows, time.monotonic() now calls QueryPerformanceCounter()which has a resolution of 100 ns. Reduce CLOCK_RES from 50 or 100 msto 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.
bedevere-bot commentedApr 30, 2024
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? Itlooks like it gives the actual clock resolution, but itactually 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... :-) |
Uh oh!
There was an error while loading.Please reload this page.
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.