Avoid timer drift during long callbacks #29023
Conversation
| # 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) |
There was a problem hiding this comment.
Under almost all circumstances you want to use perf_counter or perf_counter_ns to track time intervals. I don't know how many years it would take to lose precision in elapsed_ms due to floating point error but you might as well head it off with perf_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 on next_interval > 0, but then the code would be doing a lot of useless math for the self._interval==0 case.
There was a problem hiding this comment.
Good call on the perf_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 took longer 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.
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.
import time
import matplotlib.pyplot as plt
fig = plt.figure()
def on_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 to
1409c6c
Compare
If we set interval in a loop, this could cause the timing to become dependent on the callback processing time.
The Tk timer would reset itself after the callback had processed to add a new interval. This meant that if a long callback was being added we would get a drift in the timer. We need to manually track the original firing time and intervals based on that.
Make sure that the interactive timers don't drift when long callbacks are associated with them.
1409c6c to
2be06d4
Compare
|
Closing in favor of doing this all in one consolidated PR: #29062 |
PR summary
Updating the
intervalorsingle_shotattributes 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