Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Avoid timer drift during long callbacks#29023
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
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.
I think the Tk part of this is fine, but I have some thoughts on the timekeeping!
# We want to adjust our fire time independent of the time | ||
# spent in the callback and not drift over time. | ||
elapsed_ms = int((time.time() - self._timer_orig_fire_time) * 1000) | ||
next_interval = self._interval - (elapsed_ms % self._interval) | ||
# minimum of 1ms | ||
next_interval = max(1, next_interval) | ||
self._timer = self.parent.after(next_interval, self._on_timer) |
richardsheridanOct 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Under almost all circumstances you want to useperf_counter
orperf_counter_ns
to track time intervals. I don't know how many years it would take to lose precision inelapsed_ms
due to floating point error but you might as well head it off withperf_counter_ns
.
Also, sometimes it may be appropriate to wait less than 1 ms to catch up to the desired time, which this branch can't handle. One could fix this by moving the math up above and branching onnext_interval > 0
, but then the code would be doing a lot of useless math for theself._interval==0
case.
greglucasOct 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Good call on theperf_counter_ns()
, just pushed that change.
Our timers shouldn't be relied upon for anything less than 1ms as we get our intervals in integer milliseconds. So I think if we want to sync to something faster than that we should punt to users and tell them to bring their own timers.
This did get me thinking though. I think (I should verify this and test it...) the other backends have a timer that will emit the callbacks and reschedule the timer right away, regardless of when the last callback finished. So I think you'd potentially have multiple callbacks running at the same time if the callback tooklonger than the timer.
i.e. in Tk here if I have a callback that takes 2.5 seconds, but my timer is 1s. I'd get function calls at (1, 4, 7, ...) But I think on the other backends I'd get (1, 2, 3, 4, ...) with a piling up of events that are all running depending on what thread they were dispatched to and whether they could be started then or not. I think that can be left for a follow-up though and the first step here is just syncing on the original requested time.
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.
Following up with some results about what the various backends currently do for callbacks that last longer than the timers with the following test script that will either print ~1/second or every 2-seconds.
importtimeimportmatplotlib.pyplotaspltfig=plt.figure()defon_timer():print(time.ctime())time.sleep(1)timer=fig.canvas.new_timer(interval=950)timer.add_callback(on_timer)timer.start()fig.canvas.start_event_loop(10)
macos: 2-seconds
tkagg: 2-seconds
qtagg: 1-second
gtk4agg: 1-second
wxagg: 2-seconds
So I can't say we have a standard right now, or if we should even try to set an expectation of one or the other cases to make things consistent.
dde7089
to1409c6c
CompareUh oh!
There was an error while loading.Please reload this page.
If we set interval in a loop, this could cause the timing to becomedependent on the callback processing time.
The Tk timer would reset itself after the callback had processedto add a new interval. This meant that if a long callback was beingadded we would get a drift in the timer. We need to manually trackthe original firing time and intervals based on that.
Make sure that the interactive timers don't drift when longcallbacks are associated with them.
1409c6c
to2be06d4
CompareClosing in favor of doing this all in one consolidated PR:#29062 |
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Updating the
interval
orsingle_shot
attributes of a timer would call the underlying timer update functions regardless of if a new value was present or not. This caused issues in animations that set the interval to the same value in each iteration of the loop and when a long-running function was taking place this would cause the timers to drift.Note that even with the interval/single_shot updates, Tk was still drifting so we need to update that repeating timer code to avoid the time it takes within the callback itself. ping@richardsheridan for thoughts/suggestions on the Tk updates.
closes#28647
alternative to#28997
Example to test with from the main issue. All backends before this produced ~300ms, all backends produce ~200ms after these updates for me.
PR checklist