1411329 - consider adjusting for drift in setInterval()

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

Chrome and edge seem to adjust for drift.  Chrome's algorithm is here:

https://github.com/whatwg/html/issues/3151#issuecomment-339000314

I tested this a bit and it works well.  I think we should consider doing it with the added protection of still clamping to 4ms, background throttling, etc.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Attempt to adjust for setInterval() drift. r=bz

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8921555 [details] [diff] [review]
Attempt to adjust for setInterval() drift. r=bz

What do you think about this patch?

One thing I am unsure of:

Consider this code:

  function burnTime() {
    let start = performance.now();
    while((performance.now() - start) < 5);
  }

  setInterval(burnTime, 0);

Today we will clamp the 0 to 4ms and then schedule the next interval from the start of the callback.  So we effectively will have no spacing between callbacks.

The way I have written the drift code in this patch we will enforce throttling and 4ms clamping after adjusting for callback time, etc.  So we would end up with 4ms delays between callbacks in this code example.

Which approach do you think is better here?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I think we should try to keep our current behavior for that case, probably.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Ok, I'll rework the patches.  That makes it match what chrome does as well.

Comment 5

[Luke]

It's not immediately obvious, but Chrome's algorithm appears to run before executing the callback, whereas Firefox's runs afterwards. Eg, testing the patch using a 1000ms interval with an artificial delay of 1500ms on the second run:

Chrome: 1000ms, 2000-3500ms, 3500ms, 4000ms, 5000ms...
Nightly+Patch: 1000ms, 2000-3500ms, 4000ms, 5000ms, 6000ms...
Expected: 1000ms, 2000-3500ms, 3504ms*, 4000ms, 5000ms...

Chrome's second run: 1000 - ((2000 - 2000) % 1000)
Firefox's second run: 1000 - ((3500 - 2000) % 1000)
Chrome's third run: 1000 - ((3500 - 3000) % 1000)
Firefox's third run: 1000 - ((4000 - 4000) % 1000)

Comment 6

[Luke]

Attachment: setinterval.patch

I took a stab at adjusting your patch. I have no idea if this approach is valid but it worked on my machine. If there's an important reason why the delay cannot be negative then I guess we'll need to store the "raw" when timestamp separately.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8925270 [details] [diff] [review]
setinterval.patch

Review of attachment 8925270 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking a stab at this, but I think we should keep the requirement that delays are non-negative.  Allowing a negative delay does not provide any faster scheduling of a runnable (we don't have a time machine), but would allow the interval to run before a setTimeout(f,0) that was scheduled before the interval fired.  I hesitate to make this kind of subtle ordering change given the age of this check.

There are some other bugs in my patch I plan to fix.  I think those will address the problem

I'm at TPAC this week, but I'll try to take a look at it.

Normally I would try to talk you through what I have in mind since more contributors is great, but changing TimeoutManager is quite risky in my experience.  Seemingly simple refactors in the past have led to weeks of fixing intermittent tests because they had race conditions that started failing.  I think this makes changes to TimeoutManager not a great "first bug" for contributing to gecko.  If you really want to brave it, though, I'm happy to mentor you through the process.  If not, I expect to look at it this week.

Thanks again!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Attempt to adjust for setInterval() drift. r=bz

This needs some more cleanup, but it passes these three tests:

https://github.com/lpd-au/web-platform-tests/blob/setinterval/html/webappapis/timers/setinterval-no-execution-delay-manual.html
https://github.com/lpd-au/web-platform-tests/blob/setinterval/html/webappapis/timers/setinterval-prevent-drift-manual.html
https://github.com/lpd-au/web-platform-tests/blob/setinterval/html/webappapis/timers/setinterval-keep-cycle-manual.html