Closed
Bug 1370025
Opened 7 years ago
Closed 7 years ago
remove TimeoutManager dummy insertion point code
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 3 obsolete files)
1.25 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
14.82 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
3.72 KB,
patch
|
ehsan.akhgari
:
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.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8874150 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b1e2bad50e679b601e9accda5bcfb4277606dbf
Assignee | ||
Comment 8•7 years ago
|
||
Last try looked good. Just updating to remove some more dead code. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0408b0e8c03a81a8caf5883b2125e6df387427ed
Assignee | ||
Updated•7 years ago
|
Attachment #8874153 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
Reordered some conditionals to optimize for the common case.
Attachment #8874152 -
Attachment is obsolete: true
Assignee | ||
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
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•7 years ago
|
Attachment #8874148 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8874149 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8874151 -
Flags: review?(ehsan) → review+
Comment 15•7 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•7 years ago
|
Attachment #8874429 -
Flags: review?(ehsan) → review+
Comment 16•7 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
Comment 17•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•