fix(nodes): hold a 1 ms timer tick for the duration of a Delay - #134
Merged
Conversation
test_delay_node_is_accurate_under_realistic_loop_pressure failed about two runs in three on Windows. It is not flaky. Measured over 20 runs, a 0.5 s delay overshot by a median of 31 ms with a worst case of 63 ms, and the values were 0, 16, 31, 47, 63 -- exact multiples of 15.6 ms, which is Windows' default system timer tick. threading.Event.wait(timeout) is backed by a condition variable and cannot resolve finer than that tick, so moving the sleep off the event loop fixed the loop-contention half of the original field bug and left the platform half in place. The node's own docstring said as much without drawing the conclusion: "precision ~1 ms on macOS/Linux". Asking Windows for a 1 ms tick around the wait takes the same measurement to a median and worst case of 16 ms -- and, more usefully, makes it deterministic instead of scattered across four tick boundaries. The test then passed ten consecutive runs. The request is process-global and costs power, because a faster tick means the CPU sleeps less deeply, so it is held only while something is waiting on it rather than for the life of the process. Concurrent delays share one request through a refcount, so the first to finish cannot drop the tick out from under the others. Every failure path -- no winmm, a refused period, an exception from the call -- degrades to the coarse tick rather than propagating: a delay that is 15 ms long is a nuisance, a delay that raises ends an experiment. Worth noting the tick still bounds every other timed wait in the app. Only Delay takes it today.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It wasn't flaky
I called
test_delay_node_is_accurate_under_realistic_loop_pressureflaky earlier. It isn't — it was failing about two runs in three on Windows, and it was right to.Measured over 20 runs, a 0.5 s delay overshot by:
Those are
0, 15.6, 31.2, 46.8, 62.4— exact multiples of Windows' default 15.6 ms system timer tick.threading.Event.wait(timeout)is backed by a condition variable and cannot resolve finer than one tick. So the original field-bug fix — moving the sleep off the congested qasync loop onto a worker thread — solved the loop contention half and left the platform half untouched. The node's own docstring said so without drawing the conclusion: "precision ~1 ms on macOS/Linux". Windows was never covered.The fix
Ask Windows for a 1 ms tick around the wait. Same measurement, after:
Roughly 2× better typical, 4× better tail, and — more useful for an instrument — deterministic rather than scattered across four tick boundaries. The test then passed 10 consecutive runs.
Also correcting the docstring, which promised a precision it only delivered on two of three supported platforms.
Why it is scoped, not global
timeBeginPeriodis process-global and costs power: a faster tick means the CPU sleeps less deeply, which matters on a laptop on battery. So it is held only while something is actually waiting on it, not for the life of the process.winmm, a refused period, an exception from the call — degrades to the coarse tick rather than propagating. A delay that is 15 ms long is a nuisance; a delay that raises ends an experiment.Scope, stated plainly
The tick bounds every timed wait in the app — the data recorder's 100 ms sampling loop, the Timer node, the runner's UI timers. Only Delay takes it today. Widening that is a deliberate decision about how much of the time GLIDER should hold a fast tick, so I have not made it here.
Tests — 9 new
tests/unit/core/test_timer_resolution.py: raise and release; overlapping holders sharing one request; release on exception; no holder-count leak across repeated use; no-op off Windows; a refused period is never released later (releasing one that was never granted would unbalance Windows' own refcount and could lower another process's tick); missingwinmmsurvivable; an exception from the call survivable; a failed load not retried.Verification
PYTHONPATH=src QT_QPA_PLATFORM=offscreen pytest tests/— 3650 passed, 4 skipped, ruff and black clean. The precision test passed 10/10 consecutive runs with-m slow.Note this test is excluded from CI (
-m "not slow"), so this changes nothing about CI signal — it makes the localpytest -m slowrun trustworthy, which is the only job it has.