Should setInterval account for execution delays to prevent drift? #3151
Comments
|
It does seem there's a conflict between the description of The description says "Schedules a timeout to run handler every timeout milliseconds." Which to me means that if the interval is 10s, and the timer is started at 4:16:09, the callback will be queued up to run at 4:16:19, 4:16:29, 4:16:39, etc. As it is, the specified implementation is just shorthand for Having the handler be called on some interval would be much more useful. I think this could be accomplished by storing the current time (start_time) in the timer when it's created (which I suspect may already be done), a counter initialized to 1, change step 5.3 to "If the repeat flag is true, then call timer initialization steps again, passing them the same method arguments, the same method context, with the repeat flag still set to true, with the previous handle set to handler, and with the counter incremented by 1." add an
|
|
Thanks for taking a look! :) I've gone a little further today and compared the behaviour in cases where the function does take longer to execute than the specified timeout (test). That solution would not match the current behaviour of Chrome and Edge (or Firefox) when the function takes longer than the timeout for several iterations. Consider this timeline for an interval of 1000ms:
The result in Chrome and Edge is: The result in Firefox is: That solution would presumably result in: I don't know if this side-effect is desirable or not. It would probably be reasonable to continue placing the burden upon the developer to ensure the function executes in less time than the timeout, allowing adjustments only for early/late starts and dumping skipped intervals. I imagine the same would happen if the interval was delayed because of some other javascript executing for a really long time, but the same principle could probably be applied to that code too. Fwiw, from my pov, only Firefox's current implementation is generating bug reports for our app... |
I think this could cause issues if someone expects the callback to have been called a certain number of times and it isn't, though I guess current implementations don't guarantee that either. |
|
We absolutely do not want to allow a single delayed setInterval execution to schedule more than one followup execution. That is, if you do |
Called a certain number of times before what event occurs? |
|
What platform are you testing on? I am seeing some poor behavior in firefox on windows only: |
|
Windows, yep. If Firefox on other platforms matches Chrome and Edge then it would be great to update the spec to match their correct behaviour. |
Well, my point is you may be seeing inaccurate timing mechanisms in firefox on windows and not the kind of adjustments you are inferring in the other platforms. Also, see @bzbarsky's concerns about doing what you are suggesting. |
|
It wasn't my suggestion to start queueing multiple followup executions, and I actually agree with his concerns. But like Yay said, the spec reads as a synonym for |
|
Changing the spec to be equivalent to: Seems reasonable to me. |
Using the Mozilla test, here is my exact output from Chrome 62 (beta) and Firefox 58 (nightly): Chrome: Firefox: Whilst Firefox does have two unexplained spikes of 43 and 37, which it's great you've opened a bug for, there is still a clear behaviour difference with Chrome having many timeouts <10ms following lateness and Firefox always maintaining >=10ms. Independent of any bugs in Firefox, Chrome does more than just schedule another execution before running the callback. In my observation it's behaving closer to (not exactly): Unless I'm completely misunderstanding something, I would prefer to see something like this as the specced behaviour, although your proposal is clearly better imo than no change at all. |
|
Just to clarify, the bug I wrote is for behavior I saw where results look more like "10 15 15 10 10 15 15". The values in the previous comment look fine to me. Its unclear to me what tolerances should be permitted for setTimeout()/setInterval() to fire early. Right now we try to allow them to fire 500us early, but otherwise they are delayed. Outliers like 43 and 47 in the previous comment are due to a busy main thread and aren't really related to this issue, IMO. Overall, though, I should say that we don't guarantee precision or accuracy of setTimeout()/setInterval(). In particular, anything running faster than requestAnimationFrame()'s 16ms time frame is going to be particularly noisy on real sites. |
|
@danelphick would you be an appropriate chromium person to request input from? |
|
Here's Chrome's logic for correcting drift without adding extra callbacks: I think we should standardize on something like this. |
Drift correction like this seems reasonable to me, but it also seems wrong that this can result in a negative delay time effectively bypassing the 4ms clamp. If we were to implement this we would likely enforce a minimum delay time of 4ms. |
|
Good point. Adding a minimum clamp of 4ms sounds fine to me as long as we do it in a way that doesn't introduce drift. |
|
Well, if someone is trying to run a setInterval() at or faster than the clamp period of 4ms, then any delay will require a drift in that case. |
|
Any app/timer that relies on the main thread never being blocked for more than 4-8ms at a time is surely already doomed... |
|
Sure, I was thinking of a case where the callback or something else occasionally runs long enough to require a <4ms delay to the next invocation even though the requested interval was >4ms. Should we skip a beat in that case? For example, if I request an interval of 10ms and the first callback runs for 19ms, is the third one scheduled for t=20ms or t=30ms? (or possibly t=23ms which would add drift) |
My inclination would be to schedule for t=23ms. While I think its ok to try to adjust for drift, I don't think we should offer any guarantee to that effect. Particularly if code is doing things like running very long callbacks which actively fights against accurate timing. Also to set expectations, I will want to get @bzbarsky's input on this as well before I commit to making any change. |
|
10ms is still really small. I find it difficult to envisage anything time-related shorter than 100ms (ie tenths of a second) going for an interval over RAF. On that scale, 4ms, when the timer doesn't run for 196-199ms, is easier to consider a drop in the bucket. The real improvement is those extra 1-96ms of potential drift prevented. The 4ms is obviously completely inconsequential for something like an hours and minutes digital clock. Would it be an option to schedule for t=23ms, but treat it as 3ms late? The next run would either catch up at t=30ms (best case) or eventually fall off and skip (worse case) eg by continuing t=42ms scheduling next at t=50ms. Presumably an extra field would be required per timer. |
|
I filed a new bug to investigate setInterval() drift correction in firefox: |
|
I think I misspoke about clamping to 4ms after adjusting for drift. Today we adjust for drift induced by time spent in the callback, but do 4ms clamping before performing that adjustment. So maybe its ok to continue doing that for a more general drift adjustment as well. Sorry for my confusion. |
|
I've had a look at the Chromium code, and this is what it seems to be doing: let callback = /* the function to run */;
let interval = /* the number of seconds to wait between calls */;
function setInterval(callback, interval) {
let next_fire_time = Date.now() + interval;
function fmod(numerator, denominator) {
return /* floating-point modulus; there is no JavaScript equivalent */;
}
function repeat() {
let now = Date.now();
let next_interval = interval - fmod(now - next_fire_time, interval);
next_fire_time = now + next_interval;
setTimeout(repeat, next_interval);
callback();
}
setTimeout(repeat, interval);
}Basically, if it fires one second early, one second will be added to the next timeout delay. If it fires one second late, one second will be removed from the next timeout delay. I think this can be accomplished by moving step 6 above step 5, adding a step between them
and changing step 5.3 to
Also, there should only be a 4ms minimum delay if the nesting level is greater than 5, so the delay could be less than that. Speaking of the 4ms minimum delay though, I didn't see that handled anywhere in the Chromium code, so even if it's handled at a higher level initially, it's possible that you could have a shorter delay if the callback runs long. |
|
This is a pretty great thread, both in the original discovery and in the implementer collaboration here. I've been watching from afar for a while, bu wanted to poke my head in just to give us a bit more direction. What I'm hoping we can achieve here is:
So far I'm not sure what the concrete proposal is, or if we're still in the archeology stage. Is it just converging on what Chrome does, or do people find that not good in some ways? |
To be strictly accurate, this should use
For a non-native implementation, would the
I created three single-page tests, based on our discussions about Chrome's algorithm, and marked them manual:
On my laptop, Firefox passes only 1, Chrome and Edge pass all three. The current spec would pass zero. I don't know how Safari fares. After a little confusion, I think there is agreement to try and bring Firefox up to match Chrome and Edge. |
It would. I haven't used |
|
@lpd-au What version of edge are you testing? I've been seeing somewhat high time variances in Edge 16 on win10 fall creators update. I wonder if they changed some things here. It seems like edge has been reducing the accuracy of their timers a bit to improve battery perf. |
|
In Edge with the Fall Creators Update using that test linked earlier I was getting consistent delays of 16-18ms on battery, but when plugged in I was getting values around 10ms with smaller delays to compensate for larger delays. All three setInterval tests linked three comments up passed both while plugged-in and while on battery. |
|
I've still been using Edge 15 on my laptop, but yes @wanderview, when I unplug the power cable the intervals get capped at 60Hz: So I would say that's not a new optimisation. I guess it falls under the same spec allowance (step 15) for capping intervals in background tabs, etc. Related: below is a fourth test for behaviour in background tabs. Obviously we couldn't actually include it in the repo since it assumes background tabs get a delay of 1s, not a "user-agent defined length of time". It passes on both Chrome and Edge, with the power cable in and out. It appears their behaviour is to (temporarily) increase the interval and adjust for drift based on that, rather than just placing a floor on each calculated interval. This does seem preferable to me; timers dealing in seconds (rather than milliseconds) ought to be treated as friendly as possible imo. Firefox, currently doing no drift correction at all, of course doesn't pass right now. |
|
@skyostil Would you be able to provide more information please about how Chrome handles the interval changing, eg when the tab moves between the foreground and background or vice versa? |
|
FWIW, I'm working to make firefox pass the three tests in #3151 (comment). Not sure if I should land the tests locally in firefox tree or are we intending to put these in web-platform-tests? |
I'd say web-platform-tests. Are you happy with the tests in their current state and do we need to submit/merge a spec PR before submitting the tests PR? |
|
@lpd-au reading https://whatwg.org/working-mode might help with any questions about the process. Ideally the standard and web-platform-tests are not out of sync. (That's ideal, in practice it might sometimes be a few days and for some features it's worse, but we're making progress to always hit the ideal.) |
|
@wanderview, or anyone else on this thread, would you be willing to help us figure out the spec modifications to make the spec match those tests? It's still not clear to me what the algorithm should be. I'm happy to transcribe it into a pull request if you wish. (Or you can help do that.) |
|
Actually, I'm not sure what to do. I was under the impression the tests in #3151 (comment) were web compatible, but it seems safari agrees with firefox. I'm somewhat slammed with service worker stuff and I have parental leave coming up. I'm not sure I can help with the spec text. I was mainly working from the tests. @hober, @othermaciej, does webkit have an opinion here? |
|
To clarify, when I thought the tests were web compatible I was thinking of simply landing the implementation change with the tests in a mozilla-specific area until the spec/WPT repos were updated. Now I'm not sure. |
|
@wanderview Can you be more specific about the question? There's lots of comments on this thread. Are you asking if we'd be willing to change our behavior? Is there a good reference for the proposed new behavior? Also tagging @rniwa and @cdumez who are more likely to know details of how our timers work. |
|
I'm asking if webkit would be open to changing setInterval() behavior to account for drift such that it passes the three test cases linked in #3151 (comment). Currently the tests suggest chrome and edge account for drift, but firefox and safari do not. |
|
@othermaciej Now that I have my laptop I found the link to the chrome code that implements their drift correction. This issue is proposing to standardize around something like this: |
|
Possibly related bug in Node.js: nodejs/node#7346 (closed by mistake) |
|
To summarize, two behaviors being discussed here are:
In (1), calling In (2), calling |
|
There are pros & cons to both approaches. If you're doing some sort of animations (e.g. clock ticks), (1) makes more sense. If you're doing some sort of polling of database, etc... then (2) makes more sense. So it really depends on what web developers are using |
|
I think #3151 (comment) is mostly accurate. At least in firefox, though, we schedule the next interval from the start of the last callback. So we effectively account for "drift" caused by execution time of the callback. I don't know if webkit does the same. |
I would argue accounting for drift as edge/chrome does benefits the animation use cases, but does not hurt the database polling use cases. Ignoring drift seems to make setInterval() objectively worse to me. |
|
It would definitely hurt polling case because you'd be polling with a smaller time interval in some cases. It would increase the network load and/or disk I/O. Having said that, polling use case can be implemented by simply chaining |
Not to focus on this use case too much, but I don't completely agree with this. Providing a mean callback interval over long periods matching the requested Another use case I have seen bugs about are people using setInterval() for "simulations". For example, consider a game that provides some "resource" over to the player periodically using setInterval(). The player can then consume this resource as they play the game/simulation. We had reports these types of sites provide less resources to users running firefox over time because of drift. Since any error is always to be later, it reduces the mean number of callbacks over long periods. Anyway, I think we are probably going to switch firefox over to match edge/chrome. |
We can agree to disagree on this point then.
Yeah, that's another argument for (1). Overall, (1) would make |
|
Reminder that this mostly affects short intervals, especially where the code doesn't finish executing in time. The drift shouldn't be as apparent with longer intervals. |
If I am reading the 'timer initialisation steps' correctly, a timer set with
setIntervalshould (after the delay) invoke the passed function then schedule another callback with the same delay. However, I'm not aware of any browser that actually does this:(Note: I'm ignoring the behaviour of background tabs and cases where the function takes longer to execute than the delay.)
In my opinion the spec should be updated to describe the behaviour of Chrome and Edge, which is clearly the intuitive behaviour for developers. Whilst the spec already notes that it is wrong to rely on the timers for precision, which will still remain the case, it should be possible to rely on
setIntervalnot to cause application-level drift in the order of minutes when the delay is as low as 1s.The text was updated successfully, but these errors were encountered: