1371664 - simplify background window timeout throttling and remove ResetTimersForThrottleReduction()

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add TimeoutExecutor::SetMinimumDelay(). r=ehsan

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make nsGlobalWindow::SetIsBackground() call new TimeoutManager::MaybeUpdateBackgroundState(). r=ehsan

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Call TimeoutExecutor::SetMinimumDelay() from TimeoutManager::MaybeUpdateBackgroundState(). r=ehsan

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove old TimeoutManager code that adjusted Timeout::When() while in background. r=ehsan

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Fix browser_timeout_throttling_with_audio_playback.js not to expect clamping in background windows using audio. r=ehsan

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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a minimum delay argument to TimeoutExecutor::MaybeSchedule(). r=ehsan

I had to change the approach a little bit so we pick up audio coming or going in a background tab.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make nsGlobalWindow::SetIsBackground() call new TimeoutManager::MaybeUpdateBackgroundState(). r=ehsan

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Pass a minimum delay to TimeoutExecutor::MaybeSchedule() based on TimeoutManager::IsBackground(). r=ehsan

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove old TimeoutManager code that adjusted Timeout::When() while in background. r=ehsan

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Fix browser_timeout_throttling_with_audio_playback.js not to expect clamping in background windows using audio. r=ehsan

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove old TimeoutManager code that adjusted Timeout::When() while in background. r=ehsan

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 18

[(no longer active)]

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...

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make nsGlobalWindow::SetIsBackground() call new TimeoutManager::UpdateBackgroundState(). r=ehsan

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Pass a minimum delay to TimeoutExecutor::MaybeSchedule() based on TimeoutManager::IsBackground(). r=ehsan

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove old TimeoutManager code that adjusted Timeout::When() while in background. r=ehsan

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

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