1370025 - remove TimeoutManager dummy insertion point code

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently TimeoutManager::RunTimeout() puts a dummy Timeout in the linked list as the "insertion point".  The idea is we must not insert a new timer in the middle of the timers we are trying to currently run.  All new timers must be inserted after our current active list.

This is necessary for two reasons:

1) If we could insert within our current list we could potentially busy loop in RunTimeout.
2) Inserting within our current list would potentially end up running the timeout without an event loop turn.  This would be an observable compat issue to script.

We already store some data on the Timeout for every RunTimeout(), though; the FiringId.  Each RunTImeout gets a unique ID.  In theory we can just look for a valid FiringId instead of the dummy insertion point.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Better optimize the single mFiringIdStack case in IsInvalidFiringId(). r=ehsan

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add TimeoutManager::IsValidFiringId() helper routine. r=ehsan

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Give the Timeouts structure a reference back to its owning TimeoutManager. r=ehsan

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Give the Timeouts structure a reference back to its owning TimeoutManager. r=ehsan

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Use FiringId validity in TimeoutManager::Timeouts::Insert() and ::ResetTimersForThrottleReduction(). r=ehsan

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove the old TimeoutManager insertion point logic. r=ehsan

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove the old TimeoutManager insertion point logic. r=ehsan

Last try looked good.  Just updating to remove some more dead code.

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8874148 [details] [diff] [review]
P1 Better optimize the single mFiringIdStack case in IsInvalidFiringId(). r=ehsan

Ehsan, this fixes a small issue where IsInvalidFiringId() did not properly handle the length=1 case of mFiringIdStack.  In particular, if there is only 1 id on the stack and it *is* equal, then we incorrectly hit the `MOZ_DIGANOSTIC_ASSERT(low != high)`.

The fix here is to handle the length=1 case completely before proceeding to the more complicated cases below.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8874149 [details] [diff] [review]
P2 Add TimeoutManager::IsValidFiringId() helper routine. r=ehsan

Ehsan, this just adds a small IsValidFiringId() around the existing IsInvalidFiringId().  I find it hard enough to read the timer code already that I think its useful to avoid ! logical operators if we can.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8874151 [details] [diff] [review]
P3 Give the Timeouts structure a reference back to its owning TimeoutManager. r=ehsan

Ehsan, this gives the Timers struct a reference back to its owning TimeoutManager.  I need this so I can call TimeoutManager::IsInvalidFiringId() from calls like Insert().  I don't love this kind of circular dependency, but since the Timeouts structs are in-place objects owned by the TimeoutManager I don't think its too bad.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Use FiringId validity in TimeoutManager::Timeouts::Insert() and ::ResetTimersForThrottleReduction(). r=ehsan

Reordered some conditionals to optimize for the common case.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8874439 [details] [diff] [review]
P4 Use FiringId validity in TimeoutManager::Timeouts::Insert() and ::ResetTimersForThrottleReduction(). r=ehsan

This is the main change in this bug.  Rather than using the dummy Timeout insertion point it changes our insertion code to use the FiringId.  A valid FiringId means we have encountered the set of actively firing Timeouts.

As described in comment 0, the main point of the dummy insertion point is to make sure we don't insert within the current set of Timeouts being processed.  Checking the FiringId allows us to achieve this same effect without creating and inserting a dummy Timeout.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8874429 [details] [diff] [review]
P5 Remove the old TimeoutManager insertion point logic. r=ehsan

This patch removes the old dummy Timeout and insertion point code.

Comment 15

[(no longer active)]

Comment on attachment 8874439 [details] [diff] [review]
P4 Use FiringId validity in TimeoutManager::Timeouts::Insert() and ::ResetTimersForThrottleReduction(). r=ehsan

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

Nice!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c25fe3b1af6
P1 Better optimize the single mFiringIdStack case in IsInvalidFiringId(). r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/229af89db178
P2 Add TimeoutManager::IsValidFiringId() helper routine. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfe3c2ea812
P3 Give the Timeouts structure a reference back to its owning TimeoutManager. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fc93531b6a
P4 Use FiringId validity in TimeoutManager::Timeouts::Insert() and ::ResetTimersForThrottleReduction(). r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/cffecce93627
P5 Remove the old TimeoutManager insertion point logic. r=ehsan

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/2c25fe3b1af6
https://hg.mozilla.org/mozilla-central/rev/229af89db178
https://hg.mozilla.org/mozilla-central/rev/fbfe3c2ea812
https://hg.mozilla.org/mozilla-central/rev/a4fc93531b6a
https://hg.mozilla.org/mozilla-central/rev/cffecce93627