1378586 - clamp setInterval() based on "nesting level" similar to setTimeout()

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we clamp our DOM timeouts to 4ms in two cases:

1. setInterval() is called
2. A nested setTimeout() is called where the nesting level is at least a certain depth.

This means these two snippets are not equivalent:

  // clamps to 4ms immediately
  setInterval(f, 0);

  // executes four 0ms delay callbacks and then clamps to 4ms
  function cb() { f(); setTimeout(cb, 0); }
  setTimeout(cb, 0);

In other browsers the clamping is applied after the "nesting level" limit is reached.  In chrome its on the 5th callback.  In edge it seems to be the 3rd callback.

It would be nice to switch to simply using the "nesting level" even for setInterval().  It would bring us more in line with other browsers and would also let us delete some code.  Its possible it might also help a bit on the MrPotatoGun site in bug 1378149.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Safari seems to clamp setInterval() on the 5th or 6th callback.

Comment 2

[Boris Zbarsky [:bzbarsky]]

That would more closely align with the spec too, though of course the spec allows all this stuff to be arbitrarily delayed.  Still, https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timer-initialisation-steps doesn't use the "repeat" boolean in determining the "timeout" value; it just uses the nesting level.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Track the nesting level on interval Timeout objects. r=farre

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Avoid Timeout mNestingLevel rollover by just limiting the value to the values we care about. r=farre

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Reorder Timeout members to improve binary packing. No functional change. r=farre

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Clamp setInterval() based on nesting value instead of always. r=farre

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2145371abc6971cbf4c98a770f010772dc3108d

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Don't force setInterval() to a min 1ms delay. r=farre

https://hg.mozilla.org/try/pushloghtml?changeset=b5e71e776aa795c3004716c56c99b561a262ba50

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5e71e776aa795c3004716c56c99b561a262ba50

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8885905 [details] [diff] [review]
P1 Track the nesting level on interval Timeout objects. r=farre

This patch makes us update mNestingLevel member on setInterval() Timeout objects.  Previously it was not set and it was not updated.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8885906 [details] [diff] [review]
P2 Avoid Timeout mNestingLevel rollover by just limiting the value to the values we care about. r=farre

While the uint32_t would only roll over once every ~49 days, lets just avoid that situation.  This patch caps the mNestingLevel at the value we care about.  There isn't really any point incrementing beyond the threshold.

This also lets us shrink the mNestingLevel field to a uint8_t.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8885907 [details] [diff] [review]
P3 Reorder Timeout members to improve binary packing. No functional change. r=farre

A bit unrelated to this bug, but to take advantage of the mNestingLevel shrinking to a uint8_t, lets reorder the Timeout members to get better packing.  This reduces the size of Timeout from 104 to 96 in opt builds.  This is just enough to get in the next lower bucket in jemalloc.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8885908 [details] [diff] [review]
P4 Clamp setInterval() based on nesting value instead of always. r=farre

Stop considering the mIsInterval member when deciding whether to clamp or not.  Just look at mNestingLevel.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8886198 [details] [diff] [review]
P5 Don't force setInterval() to a min 1ms delay. r=farre

We also used to limit setInterval() delays to a minimum of 1ms.  Apparently we used a 0ms delay as a signal somewhere in the code in the distant past.  AFAICT, though, we no longer do this.  We use mIsInterval instead now.

Stop forcing a minimum 1ms delay on setInterval.

Comment 14

[Andreas Farre [:farre]]

Ben, is it ok if I get to this Monday/Tuesday next week? I'm on pto this week.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andreas Farre [:farre] from comment #14)
> Ben, is it ok if I get to this Monday/Tuesday next week? I'm on pto this
> week.

Yea, I didn't expect a response this week.  I'm going on PTO tomorrow, so just filing before I leave.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I think I should write a test before landing this.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Add a mochitest that verifies timeout clamping behavior. r=farre

This test attempts to verify we start clamping in both setTimeout() chains and setInterval() at the same iteration.  In addition, it tries to verify that we continue clamping for iterations after that.

I verified the setInterval() test steps fail without the other patches in this bug.

Note, this will be somewhat timing dependent.  I increased the clamp time pref to try to offset that.  I also preemptively disabled the test on android debug since its an agonizingly slow platform and would probably have intermittent failures.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7168ff42bb0c468d69e1f3fb762f6eed0578df8

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef429397347b
P1 Track the nesting level on interval Timeout objects. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/adc6dc3c8c4e
P2 Avoid Timeout mNestingLevel rollover by just limiting the value to the values we care about. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1d8507fc97
P3 Reorder Timeout members to improve binary packing. No functional change. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbccc5bd7c14
P4 Clamp setInterval() based on nesting value instead of always. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/81c36a4d6b36
P5 Don't force setInterval() to a min 1ms delay. r=farre
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9485e6290b
P6 Add a mochitest that verifies timeout clamping behavior. r=farre

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef429397347b
https://hg.mozilla.org/mozilla-central/rev/adc6dc3c8c4e
https://hg.mozilla.org/mozilla-central/rev/9d1d8507fc97
https://hg.mozilla.org/mozilla-central/rev/fbccc5bd7c14
https://hg.mozilla.org/mozilla-central/rev/81c36a4d6b36
https://hg.mozilla.org/mozilla-central/rev/bb9485e6290b

Comment 20

[Chris Mills [:cmills]]

I've had a go at documenting this, although you might want to check the description. I first updated the description of the behaviour on the setTimeout() page:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout#Timeouts_throttled_to_%3E4ms

I also added a note to the Fx56 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/56#DOM

Let me know if more work needs to be done on this. Thanks!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Chris, that looks good!  The only comment I have is its unclear to me if chrome/safari/edge ever clamped immediately on setInterval() or if they always did what they do today (and what we just implemented).  If you wanted you could rearrange it a bit to say browsers clamp after their given number of iterations whether via setTimeout() chaining or setInterval().  In the past some browsers clamped immediately on setInterval(); for example Firefox 55 and earlier.

But I'm not sure that nuance is really necessary or not.

Thanks!

Comment 22

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #21)
> Chris, that looks good!  The only comment I have is its unclear to me if
> chrome/safari/edge ever clamped immediately on setInterval() or if they
> always did what they do today (and what we just implemented).  If you wanted
> you could rearrange it a bit to say browsers clamp after their given number
> of iterations whether via setTimeout() chaining or setInterval().  In the
> past some browsers clamped immediately on setInterval(); for example Firefox
> 55 and earlier.
> 
> But I'm not sure that nuance is really necessary or not.
> 
> Thanks!

I *think* I understand this ;-)

Is the update any better:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout#Timeouts_throttled_to_%3E4ms

?

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #22)
> I *think* I understand this ;-)
> 
> Is the update any better:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/
> setTimeout#Timeouts_throttled_to_%3E4ms
> 
> ?

I think the current location of this text is wrong/confusing now: "Gecko acquired this behaviour in version 56."  We only really changed setInterval() in that version.

Also, to really nitpick I think the text here is a bit confusing:

"Gecko clamps on the 4th call"

Does "call" there mean the call to setTimeout() or the call to the callback?  In old versions of the text it might have meant setTimeout(), but that doesn't make as much sense now that we are including setInterval() here.  setInterval() is only called once.

This only really matters if we want the number to be exactly accurate, though.  This test shows that we clamp on the 5th callback:

http://searchfox.org/mozilla-central/source/dom/base/test/test_timeout_clamp.html

I converted this into a glitch snippet here:

https://timeout-clamp.glitch.me/

It shows that both firefox and chrome start clamping on the 5th callback currently.

Safari seems to clamp on the 6th callback.

Edge seems to introduce more delay now and often has 4ms or more delay on the first callback.  Sometimes, though, it shows that the first 4ms callback comes much, much later (11th callback, etc).  I'm not sure this is strictly clamping.  I think this might be more timeout coalescing to improve battery time.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

At the end of the day, this stuff is likely to keep changing.  As can be seen by edge, browser vendors are pushing timeouts to try to improve battery efficiency.  Sites should really not depend on timeouts less than 4ms in length in general.

Comment 25

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #24)
> At the end of the day, this stuff is likely to keep changing.  As can be
> seen by edge, browser vendors are pushing timeouts to try to improve battery
> efficiency.  Sites should really not depend on timeouts less than 4ms in
> length in general.

Thanks Ben. I've made one more set of changes to get rid of the confusion you cited above. I think I'll leave it be for now and we'll see what happens in the future.