Closed
Bug 1371664
Opened 7 years ago
Closed 7 years ago
simplify background window timeout throttling and remove ResetTimersForThrottleReduction()
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 9 obsolete files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
It would be really great to remove ResetTimersForThrottleReduction(). Its very expensive and adds some requirements on our list data structure. For example, it makes it very hard to switch to something like a binary heap. If we simplify how we do background throttling, though, we can probably remove it. Today we adjust the When() TimeStamp on each Timeout scheduled while a window is in the background. When the window comes to the foreground we have to run the expensive ResetTimersForThrottleReduction() to fix them all. As an alternative we could avoid adjusting the When() TimeStamp on each Timeout. Instead we would set a "minimum delay" on the TimeoutExecutor. This would force the TimeoutExecutor to delay at least that long whenever it would schedule a new runnable/nsITimer. When a window goes to the background it sets the TimeoutExecutor minimum delay to 1 second. When a window comes to the foreground it sets the minium delay back to 0 seconds. This would also align timers a bit within a window which is closer to what other browsers do for background windows. Its not complete alignment, though, since different windows can still run unaligned with each other.
Assignee | ||
Updated•7 years ago
|
Summary: simply background window timeout throttling and remove ResetTimersForThrottleReduction() → simplify background window timeout throttling and remove ResetTimersForThrottleReduction()
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79b86ea09d664ca9252a1c355ecd7eff960afbc7
Assignee | ||
Comment 5•7 years ago
|
||
Another advantage of doing it this way is that we will begin throttling timers immediately when the page goes in the background. Even existing timers will be throttled. Our old code would only start throttling new timers. So a misbehaving page could still do a lot of work if it had a bunch of timers set to fire.
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41bce9fe804f985a851d6043c39216a51d8bd0fc
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 7•7 years ago
|
||
I had to change the approach a little bit so we pick up audio coming or going in a background tab.
Attachment #8876154 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=509148dcdee88f59fef2517ea0f66787e12f219c
Attachment #8876246 -
Attachment is obsolete: true
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8876268 [details] [diff] [review] P1 Add a minimum delay argument to TimeoutExecutor::MaybeSchedule(). r=ehsan Ehsan, this patch adds a "minimum delay" parameter to TimeoutExecutor::MaybeSchedule(). If its non-zero then it will force the use of an nsITimer for at least the given duration. I ended up using an arg instead of a stateful SetMinimumDelay() because TimeoutManager really needs to continuously check the background value. This is necessary to pick up any changes to the audio context state.
Attachment #8876268 -
Flags: review?(ehsan)
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8876269 [details] [diff] [review] P2 Make nsGlobalWindow::SetIsBackground() call new TimeoutManager::MaybeUpdateBackgroundState(). r=ehsan This is a refactoring patch to make nsGlobalWindow tell us when the window changed background/foreground state.
Attachment #8876269 -
Flags: review?(ehsan)
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8876270 [details] [diff] [review] P3 Pass a minimum delay to TimeoutExecutor::MaybeSchedule() based on TimeoutManager::IsBackground(). r=ehsan Make the TimeoutManager pass a minimum delay to TimeoutExecutor::MaybeSchedule. This is based on the current IsBackground() state. The code will also reschedule the executor when the window changes foreground/background state to pick up any changes to the minimum delay.
Attachment #8876270 -
Flags: review?(ehsan)
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8876291 [details] [diff] [review] P4 Remove old TimeoutManager code that adjusted Timeout::When() while in background. r=ehsan This patch removes the old code that applied background throttling to each Timeout::When() value. It also removes the ResetTimersForThrottleReduction code.
Attachment #8876291 -
Flags: review?(ehsan)
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8876272 [details] [diff] [review] P5 Fix browser_timeout_throttling_with_audio_playback.js not to expect clamping in background windows using audio. r=ehsan This patch fixes browser_timeout_throttling_with_audio_playback.js to not use dom.min_timeout_value. That pref is actually only for timeout clamping, not all timeouts. I've filed bug 1371787 to fix that confusing name. Instead, this test just tries to do a 10ms timeout and verifies that its not throttled with active audio.
Attachment #8876272 -
Flags: review?(ehsan)
Comment 18•7 years ago
|
||
Comment on attachment 8876269 [details] [diff] [review] P2 Make nsGlobalWindow::SetIsBackground() call new TimeoutManager::MaybeUpdateBackgroundState(). r=ehsan Review of attachment 8876269 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/TimeoutManager.cpp @@ +1152,5 @@ > }); > } > > +void > +TimeoutManager::MaybeUpdateBackgroundState() Why not call this UpdateBackgroundState()? The function isn't doing its work only some of the times, it just happens that when you're in the foreground there is no work to do...
Attachment #8876269 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876268 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876270 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876291 -
Flags: review?(ehsan) → review+
Updated•7 years ago
|
Attachment #8876272 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8876269 -
Attachment is obsolete: true
Attachment #8877377 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8877378 -
Flags: review+
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8876291 -
Attachment is obsolete: true
Attachment #8877379 -
Flags: review+
Comment 22•7 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1f13b62ce5 P1 Add a minimum delay argument to TimeoutExecutor::MaybeSchedule(). r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c4d00409eb P2 Make nsGlobalWindow::SetIsBackground() call new TimeoutManager::UpdateBackgroundState(). r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/2645242f8171 P3 Pass a minimum delay to TimeoutExecutor::MaybeSchedule() based on TimeoutManager::IsBackground(). r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/c8facbc2dca1 P4 Remove old TimeoutManager code that adjusted Timeout::When() while in background. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/5a73c211f58d P5 Fix browser_timeout_throttling_with_audio_playback.js not to expect clamping in background windows using audio. r=ehsan
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3e1f13b62ce5 https://hg.mozilla.org/mozilla-central/rev/f3c4d00409eb https://hg.mozilla.org/mozilla-central/rev/2645242f8171 https://hg.mozilla.org/mozilla-central/rev/c8facbc2dca1 https://hg.mozilla.org/mozilla-central/rev/5a73c211f58d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
•