Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setInterval keeps drifting over time #21822

Closed
cTn-dev opened this issue Jul 15, 2018 · 12 comments
Closed

setInterval keeps drifting over time #21822

cTn-dev opened this issue Jul 15, 2018 · 12 comments
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@cTn-dev
Copy link

cTn-dev commented Jul 15, 2018

  • Version: 10.6.0
  • Platform: Windows 10 x64
  • Subsystem: timers

Hi, first i should point out that this behavior doesn't match what is happening in the v8 inside of latest chrome (67.0.3396.99).

code demonstrating the issue:

const start = Date.now();
var counter = 1;
setInterval(() => {
    var driftT = Date.now() - start - (counter * 1000);

    // do some work
    for (var i = 0; i < 100000; i++) {
        var d = new Date();
    }

    // log the drift from the start (this should be fluctuating around ~0)
    // you can run this in chrome, edge, etc (it does keep drifting in firefox)
    // in nodejs it keeps growing
    console.log(`STEP: ${counter}, Drift: ${driftT}`);

    counter++;
}, 1000);

The time compensation for setInterval changed drastically between 8.x and 10x (while 10x being more proper).
However setInterval still appears to be slipping over time.

output from the above code:

STEP: 1, Drift: 1
STEP: 2, Drift: 5
STEP: 3, Drift: 2
STEP: 4, Drift: 3
STEP: 5, Drift: 4
STEP: 6, Drift: 5
STEP: 7, Drift: 6
STEP: 8, Drift: 6
STEP: 9, Drift: 6
STEP: 10, Drift: 6
STEP: 11, Drift: 6
STEP: 12, Drift: 6
STEP: 13, Drift: 6
STEP: 14, Drift: 6
STEP: 15, Drift: 6
STEP: 16, Drift: 7
STEP: 17, Drift: 8
STEP: 18, Drift: 8
STEP: 19, Drift: 9
STEP: 20, Drift: 9
STEP: 21, Drift: 9
STEP: 22, Drift: 9
STEP: 23, Drift: 9
STEP: 24, Drift: 9
STEP: 25, Drift: 10
STEP: 26, Drift: 11
STEP: 27, Drift: 11
STEP: 28, Drift: 11
STEP: 29, Drift: 11
STEP: 30, Drift: 12

This shows a drift of 12ms over the 30 second testing period.

In production i am using setInterval with 60 000ms delay, but since the system tends to have very large uptime, slippage from the starting point is starting to become an issue.

@lpinca lpinca added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jul 15, 2018
@lundibundi
Copy link
Member

lundibundi commented Jul 15, 2018

Afaik we had #7346 but didn't implement any drift-prevention mechanisms.

This is still not fixed in a few browsers as there is no spec/agreement on the best solution. Currently, Chrome and Edge seem to correct the interval for each next execution (aligns with the results you are getting) but Firefox and reportedly Safari doesn't hence you should get the same results as with node.

Related to whatwg/html#3151.
Interesting comments from the issue.
Chromium approach and this.
possible tests
and a few comments at the end.

Personally, I think it'd be better to implement something similar to Chromiums approach to fix this (fix the drift before/while rearming the interval).

@cTn-dev
Copy link
Author

cTn-dev commented Jul 15, 2018

Correct, i actually did a deep dive through most setInterval related bug tickets before opening this one, since most of them got eventually closed and haven't really came to any definite conclusion and had no activity on them for 6+ months.

I was personally really surprised by the different behavior.
Not accounting for the overhead prior to the rearm seems counter intuitive.

In node uptime of weeks/months is completely normal for a node server, when it comes to websites users don't typically spend that amount of time on a single page so drift of couple of ms isn't a big deal there.
On the other hand if we consider a drift of ~1ms per interval firing, such drift accumulated over the span of 30 days doesn't sound very healthy to me.

Considering setInterval is just a "neat" way to setup re-curing timeout, doesn't such "drifty" behavior defeat the purpose of having it in the first place?
When a user (like me) have to write their own "fixed" setInterval to do the exact same thing just more accurately.

Another great example that completely confuses everyone is setInterval in NW.js

  1. in separate context, the user javascript will run with the fixed setInterval from chromium
    the setInterval in node context will most likely drift? (even i don't know)
  2. in mixed context, it will use the node implementation of setInterval (will drift)

When it comes to electron i have no idea, but as that is mixed context by default, so will most likely drift.
I guess you see where i am going with this, the behavior is all over the place while all of it runs on v8.

The lack of spec is a big problem here, but if this isn't going to change, i think it would be worth adding a note about the drift in the timers section, as judging by the previous bug tickets i am not the only one that fell into this trap, some "official" stance on this would go a long way.

@apapirovski
Copy link
Member

Drift correction is a very problematic topic. E.g., chromium corrects for drift but only if it's less than some arbitrary threshold for each run. If it exceeds that then it just ignores the drift altogether and behaves worse than Node.

There are also loop blocking concerns when it comes to drift because it's possible to end up in a world where the interval just stalls the loop forever. The logic has to be quite complex which in my opinion isn't worth the effort.

If one were to look at Linux timers, those don't even guarantee to be executed on the correct order of milliseconds for long-running timers.

@antsmartian
Copy link
Contributor

Drift correction is a very problematic topic. E.g., chromium corrects for drift but only if it's less than some arbitrary threshold for each run. If it exceeds that then it just ignores the drift altogether and behaves worse than Node.

Exactly. I ran the issue code on my chromium, it behaves so weird. In a new empty tab, it almost do the drift correction, but where the page has few animations/updates running it behaves more worse -- drift correction completely stops.

@davisjam
Copy link
Contributor

Here is my understanding of the cause of the behavior described in this issue. I am stating it here for anyone who wants to debate or investigate a fix. No guarantee that I'm correct in my observations.

High level

setInterval is implemented by registering a timer. After that timer is scheduled, it is rearmed to go off again after the same time interval T.

Timers are not guaranteed to go off precisely when they are scheduled, for example because the event loop is busy doing other things. Each time the timer is late there is drift D. The logic to re-arm the timer is unaware of the drift and schedules it to run again after time interval T -- so it will go off after T + D relative to when the timer previously went off. This drift is cumulative: the timer will not get closer to the expected expiration time than it was in the previous iteration (as long as the system clock is consistent), but it may continue to drift.

I see two possible forms of drift, assuming the underlying system clock is accurate.

  1. Node might schedule timers later than they were requested (e.g. due to contention on the event loop). This would affect all timers uniformly.
  2. Node might count scheduling time at the conclusion of the timer callback rather than when it expired. If so, then for example a timer scheduled every 100 ms whose callback takes 50ms will be scheduled at times 0, 150, 300, 450, etc. rather than times 0, 100, 200, etc.

Implementation specifics

See lib/timers.js.

  • TImers live in TimersLists. These lists execute their timers in listOnTimeout by calling tryOnTimeout on each ready timer.
  • On timeout (tryOnTimeout), collect the current time in start before executing the callback (thus attempting to discount the cost of the callback itself). Then rearm(timer, start) is called to reschedule the timer.
  • For repeating timers, rearm calls insert, having set the timer's _idleTimeout with the interval length, and propagating the start value.
  • insert:
    • Save the start (taken before the timer's callback was executed) in timer._idleStart.
    • If an existing TimersList has interval _idleTimeout, add the timer there. Note that this ignores the start value, meaning that the timer will typically drift with, at minimum, the cost of its callback.
    • If no such TimersList exists, create one with earliest expiry time start + interval and interval whatever the timer was set to. Thus, in this case we do discount the cost of the timer callback.
  • Back in listOnTimeout, we use timer._idleStart to check whether we are too early to handle each timer in the list.

Comments

  1. It appears that the drift @cTn-dev describes is inevitable as part of the implementation. Expired timers are re-inserted into the list from whence they came, e.g. the "go off in 100 ms" list, whether or not they are already 50 ms into the next 100 ms interval.
  2. It looks to me like both forms of drift will occur. (1) If a timeout is executed later than scheduled it is not scheduled any earlier next time; and (2) The cost of the timeout callback is ignored when choosing the list for insertion.

In principle, time checks (see getLibuvNow) could be used to guide an expired timer into the right list in insert. With this logic, the same timer might be placed in list T after one interval and list T-5 after another interval, depending on how much drift it encountered in that interval.

Doing this would require introducing more TimersLists! At the moment, when many timers use the same interval, they all enter the same list -- say, 1000 timers all in the same 100 ms list. But if we take drift into account, we might instead have these 1000 timers in lists between 85 and 100. More generally I would expect that if a Node.js process has N TimersLists, then such a scheme would inflate it to have k*N TImersLists, where k is the maximum expected drift in a given interval.

If this approach were taken I would want to see overheads measured:

  • for the extra calls to getLibuvNow
  • for maintaining the extra k*N TImersLists.

Thoughts on other designs?

Comparison to libuv

It is worth noting that libuv's timer implementation has similar properties. Timers are re-armed before executing their callbacks (see uv__run_timers), which addresses drift due to the cost of the callback (type 2). However, there is no attempt to address drift due to timers being executed later than scheduled (type 1).

@Fishrock123
Copy link
Contributor

I don't think we'll ever "fix" type 1. If the loop is busy, your timer will not be called until the queue is empty.

I don't see this changing, maybe there is some way to make a user library with threads and V8 interrupts that would get around it.


As for type 2... I think I remember this being discussed in the past. I don't remember the specific of what was discussed and changed or not, though.

k*N TimersLists

This is actually a pretty significant regression, particularly if this also had to be applied to http timeouts. We aren't guaranteed how fast object-map (or Map) property access is, but iirc it seemed to be sub- O(n) (at least in non-extraordinary cases), still, the more items we add the slower it'l become. Considering that node timers efficiency relies on this property of common-case same-duration timeouts, we should probably avoid that if possible.

@Fishrock123
Copy link
Contributor

(Also, if there is a way to design a user library that in some way can better adjust to re-scheduling timers more correctly but at an added cost, that may also be a more appropriate route to fix type 2.)

@JamesNewton
Copy link

Can I just point out that as an embedded programmer who has been looking to Node.JS as a direction to move in my work, a setInterval that drifts is... a cause for pause. I guess I was hoping it was more like an interrupt being fired in an embedded controller. That is quite possible not important to most of you, but I thought I might raise that point for us embedded guys.

@devsnek
Copy link
Member

devsnek commented Mar 26, 2019

aside from timers, javascript doesn't have interrupts, so don't expect to see anything like interrupts in the javascript ecosystem.

@JamesNewton
Copy link

Yes, I understand it's not actually doing interrupts. And yet it is "event driven" which is very much interrupt adjacent. The distance between those two concepts is positive, but not large. In my (never) humble opinion. The point is that where I would have a timer interrupt in an embedded device which might do something every minute, I would expect to be able to setInterval in Node.JS and over the course of an hour, both should do the function 60 times. Exactly 60 times.

@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

There's been no further activity on this for over a year. With the understanding that there's likely some improvements that could potentially be made, those won't happen unless someone has a concrete proposal in the form of a pull request. Going to close the issue for now but can reopen if necessary

@cedric-h
Copy link

cedric-h commented Apr 9, 2022

For anyone stumbling across this issue, I played with this some today and ended up with this script:
Obviously it's more resource intensive than the setInterval equivalent, but for when the absence of drift REALLY matters
(maybe, for example, you're stepping physics in a web game, where drift manifests as jerky entity motion)

const start = performance.now();
let tick = 0;
(function step() {
  setTimeout(step, 0);

  const elapsed = () => performance.now() - start;
  while (elapsed() / 1000 > tick) {
    let driftT = elapsed() - (tick * 1000);
    console.log(`tick:     STEP: ${tick}, Drift: ${driftT}`);

    tick++;
  }
})();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

No branches or pull requests