Closed Bug 1385453 Opened 7 years ago Closed 7 years ago

Make it possible for async tab switcher to "warm up" tabs that are likely to be displayed soon

Categories

(Firefox :: Tabbed Browser, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Status
firefox57 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance][qa-commented])

Attachments

(7 files, 1 obsolete file)

The async tab switcher tells the content process to paint and upload layers to the compositor once the switch is requested by setting the setDocShellIsActive property on the remote browser to true.

The async tab switcher is already prepared to deal with multiple layer trees hanging around - when we switch away from a tab, we don't unload the layer trees (set setDocShellIsActive to false) until a timeout has expired. That way, we can switch back and forth between tabs more quickly.

Like the speculative connection stuff in Necko, I'd like to make it possible to speculatively warm up the layer tree for a tab that we think is likely to be switched to.

Simplest scenario would be to warm up the layer tree when a tab is hovered. We can keep the same rules for unloading the layer tree (on a timer).

We could then do a bit better in bug 1355426 - instead of just switching to the next tab before closing the requested tab, we could identify and warm up the layers of the next tab when the close icon is hovered.

Similar tricks could be done with the keydown events for the Cmd/Ctrl-W and tab switching keyboard shortcuts.
Cc'ing you billm, just so this is on your radar. I'm likely to request review from you when I've got a patch.
I think we should do this - it'll (hopefully) open up a number of avenues to improve perceived tab switching and tab closing times.
Assignee: nobody → mconley
Whiteboard: [photon-performance]
Status: NEW → ASSIGNED
Iteration: --- → 56.4 - Aug 1
Flags: qe-verify?
Priority: -- → P1
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Would this affect zombie tabs? 

I notice that our memory footprint and CPU impact when idle is dramatically worse when more tabs are awake — page code gets to do stuff, and it rapidly adds up to a power-hungry 15% CPU. I'd hate to see un-measured regressions in the large-session workflows we recently touted in the press as a side effect of responsiveness work.
(In reply to Richard Newman [:rnewman] from comment #3)
> Would this affect zombie tabs? 
> 

It'll mean that we'll speculatively wake up these tabs, but if the user doesn't switch to them, we'll unload them (and put them back to sleep) after some number of milliseconds. The way I'm picturing this, even if a bunch of tabs have been warmed up, if the user goes idle and doesn't trigger any actions to cause warm ups, we'll put the background tabs to sleep in short order.

We should probably cap the number of tabs we can wake up though. We might need to tune some stuff here.
Okay, here's what I'm thinking for a design of this thing:

1) Expose a method on tabbrowser for warming up tabs, taking a tab as an argument. That method should just bail out if the tab isn't for a remote browser. Otherwise, call _getSwitcher().warmUpTab(tab).

2) Switcher should have an Array on it for storing tabs that we're warming up. We'll use that Array as a FIFO clamped warm-up list for which tabs we're warming up. Not sure what to clamp it at yet, so let's say 3 for now to pick a number out of the air.

3) If the tab's browser is already in the warm-up list, bail out - nothing to do.

4) Otherwise, add it to the warm-up list, and set the tab's state to STATE_LOADING. If we've overflowed the warm-up list, take the first entry in the list, remove it, and set it to STATE_UNLOADING. Also set a X second timer that will call onWarmupTimeout (if a warm up timer already exists, cancel it) Not sure what to set X for now, so let's say 2 seconds.

5) If onUnloadTimeout is ever fired during this window of time, skip unloading any tabs that are in the warm-up list. If a browser is requested during this window of time, remove it from the warm-up list. If onLayersReady, onLayersCleared, onRemotenessChange, or onSwapDocShells is called, remove the associated browser from the warm-up list.

6) Once onWarmupTimeout is hit, clear out the warm-up list, and set each tab in that list to STATE_UNLOADING. If there's no onUnloadTimeout timer queued, queue one up.

7) In finish(), we should assert that the warm-up list is empty.

I think that'll give us what we want. Unfortunately, it adds a bit more complexity to what is already a complex async tab switcher.

What do you think of this billm? Any holes?
Flags: needinfo?(wmccloskey)
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
I guess it's weird to me that we would have two separate mechanisms for caching tabs in this scheme. The only differences are:
- The timeout for unloading them. The existing timeout is 300ms, and the new scheme needs a longer timeout.
- The new scheme would only allow 3 cached tabs (or some number). That's probably something that would be useful in the old scheme as well.

I think it would be nice to extend the current timeout scheme so that it works for your needs. We could use a single FIFO queue for all tabs in STATE_LOADED. We would cap the size at 3 or whatever and unload the least recent tab.

For the timeout, I think it would be fine to use the exact same mechanism as now. The only difference is that we would use a different timeout based on the method of loading. requestTab would use 400ms. warmUpTab would use 2s. In both cases, an existing timeout would be cleared (giving all loaded tabs a temporary reprieve).

I think this would be simpler and it seems like it would still achieve your purposes.
Flags: needinfo?(wmccloskey)
Comment on attachment 8895554 [details]
Bug 1385453 - Add Telemetry to measure how successful speculative tab warming is when switching tabs. , data-review=liuche

Hey liuche, got a second to give this a data-review?

I'm collecting whether or not a tab that a user selects has been speculatively "warmed up" or not, and if so, recording "how warm" it was (still loading or fully loaded). This is an opt-out probe, just like the rest of our tab switch probes.

Sound okay?
Attachment #8895554 - Flags: review?(liuche)
Comment on attachment 8895554 [details]
Bug 1385453 - Add Telemetry to measure how successful speculative tab warming is when switching tabs. , data-review=liuche

https://reviewboard.mozilla.org/r/166762/#review172954

data-review+ only.

Looks good to me, this is collecting Type 1 interaction data about tabs, and it looks like this would clearly help learn more about how to improve tab-switching, so release opt-out is okay.
Attachment #8895554 - Flags: review?(liuche) → review+
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Comment on attachment 8895552 [details]
Bug 1385453 - Add API to tabbrowser to speculatively warm-up tabs for tab switching.

https://reviewboard.mozilla.org/r/166758/#review175080

r- because of the assertion issue below.

One idea here: it seems like the sole purpose of the warmingTabs array is to allow us to discard tabs so we don't keep too many around. In keeping with my suggestion in comment 6, I think it would be good to generalize that mechanism beyond warmed tabs. A simple way to do that would for postActions() to count how many tabs in the LOADING/LOADED state we have. If there are too many, it could call onUnloadTimeout immediately to flush everything. This would be a bit aggressive, but I suspect it would work fine most of the time.

::: browser/base/content/tabbrowser.xml
(Diff revision 2)
> -              if (this.blankTab) {
> -                this.maybeFinishTabSwitch();
> +              this.maybeFinishTabSwitch();
> -              }

If you make this change, I think you can probably get rid of most of the other maybeFinishTabSwitch calls.

::: browser/base/content/tabbrowser.xml:4689
(Diff revision 2)
> +                  tab === this.requestedTab ||
> +                  tab === this.visibleTab) {

Not sure these conditions are necessary given the LOADING/LOADED checks below, but I guess they can't hurt.

::: browser/base/content/tabbrowser.xml:4721
(Diff revision 2)
> +              }
> +
> +              this.logState("warmupTab " + this.tinfo(tab));
> +
> +              this.warmingTabs.push(tab);
> +              this.setTabState(tab, this.STATE_LOADING);

It seems like you're missing the displayport suppression logic here. Could we try to share more code with requestTab? I also worried about not calling preActions/postActions.

::: browser/base/content/tabbrowser.xml:4725
(Diff revision 2)
> +                this.assert(evicted !== this.requestedTab);
> +                this.assert(evicted !== this.visibleTab);

These are good things to assert, but what guarantees these assertions? Suppose I hover over a tab and then click on it. It loads. Then, before the tab switcher deactivates, I hover over a few more tabs. Couldn't we try to unload the active tab?
Attachment #8895552 - Flags: review?(wmccloskey) → review-
Comment on attachment 8895553 [details]
Bug 1385453 - Speculatively warm-up a tab for the async tab switcher when hovering the tab element.

https://reviewboard.mozilla.org/r/166760/#review175090
Attachment #8895553 - Flags: review?(wmccloskey) → review+
Comment on attachment 8895554 [details]
Bug 1385453 - Add Telemetry to measure how successful speculative tab warming is when switching tabs. , data-review=liuche

https://reviewboard.mozilla.org/r/166762/#review175096

Hmm, I guess we would still need warmingTabs for this. Unless we just have it check the state, but that might count tabs that were loaded in some other way.
Attachment #8895554 - Flags: review?(wmccloskey) → review+
Comment on attachment 8895554 [details]
Bug 1385453 - Add Telemetry to measure how successful speculative tab warming is when switching tabs. , data-review=liuche

https://reviewboard.mozilla.org/r/166762/#review175096

So how should we roll here? Forget the Telemetry, use warmingTabs, or a mixture?
ni?ing for comment 18
Flags: needinfo?(wmccloskey)
Comment on attachment 8895552 [details]
Bug 1385453 - Add API to tabbrowser to speculatively warm-up tabs for tab switching.

https://reviewboard.mozilla.org/r/166758/#review175080

> Not sure these conditions are necessary given the LOADING/LOADED checks below, but I guess they can't hurt.

Yeah, you're right - I'll remove the redundancy.

> These are good things to assert, but what guarantees these assertions? Suppose I hover over a tab and then click on it. It loads. Then, before the tab switcher deactivates, I hover over a few more tabs. Couldn't we try to unload the active tab?

> Suppose I hover over a tab and then click on it. It loads. Then, before
> the tab switcher deactivates, I hover over a few more tabs. Couldn't we
> try to unload the active tab?

Okay, let's consider that scenario. We hover that first tab, and it gets put into the warming array. Then we click on it, and so we enter `requestTab`, which calls `unwarmTab`, which then removes it from the warming array since we're about to show it.

The switcher is still alive, and we hover a few other tabs. The active tab is not in the warming array, so whatever is being evicted from that array can't be the active tab.

That's how I see it anyhow. Dropping, but feel free to re-open the issue if there's something I'm missing.
OK, you're right, I think the assertions should not fire.

I have a new concern, though. Won't this patch allow us to change the state of a tab directly from STATE_LOADING to STATE_UNLOADING (when dropping a warmingTab). I remember that, originally, that was never supposed to happen. Maybe things have changed with the tab switcher that make it okay, but it was originally a pretty bad bug. I think the problem was that the parent and child would no longer be in sync about the state of a tab. The parent could get the onLayersReady event when the tab was in STATE_UNLOADING. Then it would switch to that tab, which would then be unloaded while it was being displayed.

In general, I really would rather avoid introducing new mechanisms if we can. It's so hard to reason about this code already.

Another compromise would be to have an array of all the tabs in STATE_LOADED. It would be managed by setTabState. Then, in postActions, you could remove tabs from this array if the meet the conditions in onUnloadTimeout and if the array is too large.

I think it would be fine to keep warmingTabs around only for telemetry purposes. We would just need a comment saying so.
Flags: needinfo?(wmccloskey)
So in this last set of patches, here's what I did:

1) I keep the warmingTabs collection (switching it to a WeakSet), and use it only for Telemetry and logging
2) I've moved "eviction" to postActions, where I synchronously call onUnloadTimeout if the number of STATE_LOADED tabs exceeds MAX_WARMING_TABS. This does mean that we're re-entering postActions, but as far as I can tell, that doesn't appear to be harmful.
3) Because eviction is being handled by postActions, we no longer have a case where we transition from STATE_LOADING to STATE_UNLOADING, which (as you rightly say in comment 21) is problematic. I had forgotten that - thanks for reminding me.
4) I've attempted to de-duplicate the code that was in warmupTab and requestTab. This means that warmupTab also gets the preActions and postActions code called on it.

How does this look?
Comment on attachment 8895552 [details]
Bug 1385453 - Add API to tabbrowser to speculatively warm-up tabs for tab switching.

https://reviewboard.mozilla.org/r/166758/#review176508

Thanks. This looks good. I think it might be nice to factor out the main body of onUnloadTimeout so that we don't call preActions/postActions twice.
Attachment #8895552 - Flags: review?(wmccloskey) → review+
Blocks: 1392815
Thanks! Filed bug 1392815 to clean-up onUnloadTimeout. I'll do that next.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d78b8bc9ae5
Add API to tabbrowser to speculatively warm-up tabs for tab switching. r=billm
https://hg.mozilla.org/integration/autoland/rev/5591ea0e73e7
Speculatively warm-up a tab for the async tab switcher when hovering the tab element. r=billm
https://hg.mozilla.org/integration/autoland/rev/0ae21c1a172f
Add Telemetry to measure how successful speculative tab warming is when switching tabs. r=billm,liuche, data-review?liuche
Blocks: 1392858
This is hilarious - apparently, during our automated tests, the mouse cursor hangs out in the top left corner of the screen, and for this test at least, it appears that it hovers over the initial tab, which causes it to warm up, and that breaks this tests assumptions.

Here's a screenshot of the test failure on Linux - notice that the initial tab has the background-hovered colour applied to it.
Flags: needinfo?(mconley)
Attachment #8900382 - Flags: review?(dtownsend)
Comment on attachment 8900382 [details]
Bug 1385453 - Move the mouse to the bottom center of the screen between test runs.

https://reviewboard.mozilla.org/r/171736/#review176994

::: testing/mochitest/browser-test.js:412
(Diff revision 1)
> +      winUtils.sendNativeMouseEvent(Math.floor(window.screen.width / 2),
> +                                    window.screen.height, null, 0, null,
> +                                    observer);

From Mossop in IRC (edited out cross-talk):

2:31 PM <@Mossop> mconley: I think I'll be asking for some changes here
2:31 PM <mconley> Mike Conley Mossop: sure, I'm all ears
2:36 PM <@Mossop> mconley: So the native message has different meanings on different platforms. You're passing null for it when really it needs to be a number and that number changes per-platform
2:36 PM mconley: Specifically it looks like 1 on windows, 3 on linux mean "motion"
2:37 PM <mconley> Mike Conley Mossop: where are you finding these values, out of curiosity?
2:37 PM under widget/ somewhere?
2:37 PM <@Mossop> mconley: Windows: https://msdn.microsoft.com/en-us/library/windows/desktop/ms646273%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396
2:37 PM mconley: Linux: https://developer.gimp.org/api/2.0/gdk/gdk-Events.html
2:37 PM <mconley> Mike Conley ohhhhh
2:37 PM okay, yeah, gotcha
2:37 PM Mossop: good call, thanks
2:38 PM <@Mossop> I'm having trouble finding the right value for OSX
2:38 PM <mconley> Mike Conley Mossop: I think I saw it earlier
2:38 PM <@Mossop> mconley: As it happens on Linux we ignore all values except for a couple of button presses and just always simulate a move in that case. But I'd like us to use the right value if that changes in the future
2:38 PM <mconley> Mike Conley Mossop: http://searchfox.org/mozilla-central/source/widget/tests/native_mouse_mac_window.xul#59
2:39 PM magic numbers everywhere
2:39 PM <@Mossop> Perfect
2:39 PM <mconley> Mike Conley Mossop: cool, will update the patch
2:39 PM thanks so much
2:40 PM <@Mossop> mconley: Some comments on what the magic numbers mean would be good
Attachment #8900382 - Attachment is obsolete: true
Comment on attachment 8900453 [details]
Bug 1385453 - Make it possible to disable tab warming.

https://reviewboard.mozilla.org/r/171816/#review177654
Attachment #8900453 - Flags: review?(wmccloskey) → review+
Comment on attachment 8900454 [details]
Bug 1385453 - Disable tab warming in browser_bug343515.js test.

https://reviewboard.mozilla.org/r/171818/#review177658
Attachment #8900454 - Flags: review?(wmccloskey) → review+
Comment on attachment 8900455 [details]
Bug 1385453 - Factor out unloading tabs helper for async tab switcher to avoid postActions re-entry.

https://reviewboard.mozilla.org/r/171820/#review177660

::: browser/base/content/tabbrowser.xml:4552
(Diff revision 1)
>  
> +            // If there are any non-visible and non-requested tabs in
> +            // STATE_LOADED, sets them to STATE_UNLOADING. Also queues
> +            // up the unloadTimer to run onUnloadTimeout if there are still
> +            // tabs in the process of unloading.
> +            unloadUnselectedTabs() {

Maybe call this unloadNonRequiredTabs?
Attachment #8900455 - Flags: review?(wmccloskey) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9d8f57aa3b73 -d 2cbd34e59b97: rebasing 415727:9d8f57aa3b73 "Bug 1385453 - Add API to tabbrowser to speculatively warm-up tabs for tab switching. r=billm"
merging browser/app/profile/firefox.js
merging browser/base/content/tabbrowser.xml
warning: conflicts while merging browser/base/content/tabbrowser.xml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee9bf0aa3a00
Add API to tabbrowser to speculatively warm-up tabs for tab switching. r=billm
https://hg.mozilla.org/integration/autoland/rev/2a6337503e6d
Speculatively warm-up a tab for the async tab switcher when hovering the tab element. r=billm
https://hg.mozilla.org/integration/autoland/rev/73150fe2d9b0
Add Telemetry to measure how successful speculative tab warming is when switching tabs. r=billm,liuche, data-review=liuche
https://hg.mozilla.org/integration/autoland/rev/6a47767575e6
Make it possible to disable tab warming. r=billm
https://hg.mozilla.org/integration/autoland/rev/76ea53d7bdf0
Disable tab warming in browser_bug343515.js test. r=billm
https://hg.mozilla.org/integration/autoland/rev/4c480935fe7f
Factor out unloading tabs helper for async tab switcher to avoid postActions re-entry. r=billm
Mike, can you please tell me on what possible regression areas should I focus for this bug?
Flags: needinfo?(mconley)
(In reply to Adrian Florinescu [:AdrianSV] from comment #53)
> Mike, can you please tell me on what possible regression areas should I
> focus for this bug?

Yeah, for sure.

This change affects tab switching. Specifically, it should increase the perceived performance of switching tabs when using the mouse (since we "warm up" a background tab when hovering it with the mouse).

We should ensure that tab switching in general hasn't regressed in some way, so some scenarios to test:

1) Switching between several tabs rapidly using the keyboard and the mouse
2) Dragging tabs between windows
3) Ensuring that hovering the mouse over a tab _does_ improve perceived tab switch time. Perhaps a good test would be to load https://testpilot.firefox.com/experiments/notes in a background tab, and compare perceived tab switching performance when switching with and without this patch.
Flags: needinfo?(mconley)
Whiteboard: [photon-performance] → [photon-performance][qa-commented]
Depends on: 1394118
This interacts with media.block-autoplay-until-in-foreground in an unexpected way.  Previously I opened tabs with videos and mouseover did not start the video.  Now mouseover starts the video.
This fired a telemetry alert[1] showing that the measured tabswitch time has gone down dramatically (the number of tab switches reported remained roughly level, and the number of reporting clients remained roughly level, so this change is almost certainly due to samples reporting lower values).

If you want the tl;dr "Show me the satisfying graph" link, it is here: https://mzl.la/2xAs96T

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1288/alerts/?from=2017-08-26&to=2017-08-26
Depends on: 1394455
Depends on: 1395735
See Also: → 1274919
Blocks: 1397426
Depends on: 1397525
Flags: qe-verify+ → qe-verify-
QA Contact: adrian.florinescu
I'm afraid tab warming had to be disabled due to the regressions it introduced. We disabled it in bug 1394455.

The API remains, and will need some re-jigging before it can be re-enabled. I plan on doing that work in bug 1397426.
No longer depends on: 1397525
Depends on: 1397525
Depends on: 1400865
See Also: → 1649957
See Also: → 1689260
You need to log in before you can comment on or make changes to this bug.