remove TimeoutManager dummy insertion point code

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

1.25 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.41 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
2.35 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
14.82 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
3.72 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
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.
Blocks: 1369494
Attachment #8874153 - Attachment is obsolete: true
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.
Attachment #8874148 - Flags: review?(ehsan)
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.
Attachment #8874149 - Flags: review?(ehsan)
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.
Attachment #8874151 - Flags: review?(ehsan)
Reordered some conditionals to optimize for the common case.
Attachment #8874152 - Attachment is obsolete: true
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.
Attachment #8874439 - Flags: review?(ehsan)
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.
Attachment #8874429 - Flags: review?(ehsan)

Updated

2 years ago
Attachment #8874148 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8874149 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8874151 - Flags: review?(ehsan) → review+

Comment 15

2 years ago
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!
Attachment #8874439 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8874429 - Flags: review?(ehsan) → review+

Comment 16

2 years ago
Pushed by bkelly@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.