841096 - about:sync-tabs from other devices is very slow to load

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Sam Tobin-Hochstadt [:samth]]

Loading about:sync-tabs either via the menu, via direct entry, or even reopening it as the most recently closed tab, is very slow.  I timed it with a stopwatch at over 12 seconds on a very fast machine (which does have a rotating disk) on FF 21 and at over 5 seconds on a slower machine with an SSD.  This is with a large number of synced tabs, but the synced tabs list opens instantly on my 2 year old phone with Fennec.

Comment 1

[Valery Yundin]

Attachment: Speed up about:sync-tabs loading time

It is indeed extremely slow for large number of synced tabs. I have ~100 synced tabs from other devices and about:sync-tabs takes almost a minute to load (that is on Core i7 with SSD disk).

I did some profiling and it looks like RemoteTabViewer._generateTabList is at fault. It calls TabEngine.locallyOpenTabMatchesURL for every remote tab. locallyOpenTabMatchesURL: is in turn calling rather expensive Session.getBrowserState().

The obvious solution is to take a list of opened tabs' urls first and then compare to them in the loop, rather then building entire browser state every time.

I'm attaching a patch which does that (it builds a dictionary localURLs from a list returned by getAllTabs to use the same syntax as already done for seenURLs).

With this changes now about:sync-tabs loads acceptably fast (~1 second for 100 synced tabs).

PS it obsoletes locallyOpenTabMatchesURL function, but I didn't remove it.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8394922 [details] [diff] [review]
Speed up about:sync-tabs loading time

Review of attachment 8394922 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for diving in with a patch! This is some horrible code, so I appreciate you taking the time.

I'd like to switch this to have TabEngine return a Set, which simplifies calling code, and then to amend tests and other calling code appropriately.

::: browser/base/content/sync/aboutSyncTabs.js
@@ +143,5 @@
> +    let localURLs = engine.getAllTabs().reduce(function(dict, tab) {
> +      let url = tab.urlHistory[0];
> +      dict[url] = null;
> +      return dict;
> +    }, {});

Replace with

  let localURLs = engine.getOpenURLs();    // A Set.

@@ +151,5 @@
>        let appendClient = true;
>        let seenURLs = {};
>        client.tabs.forEach(function({title, urlHistory, icon}) {
>          let url = urlHistory[0];
> +        if (url in localURLs || url in seenURLs)

if (!url || url in seenURLs || localURLs.has(url)) {

::: services/sync/modules/engines/tabs.js
@@ +82,3 @@
>     * that as soon as you navigate anywhere, the original tab will
>     * reappear in the menu.
>     */

This comment is no longer accurate. Kill or fix, please!

@@ +82,5 @@
>     * that as soon as you navigate anywhere, the original tab will
>     * reappear in the menu.
>     */
> +
> +  getAllTabs: function TabEngine_getAllTabs() {

No need for the function name.

But as this is new, why not:

  /**
   * Return a Set of open URLs.
   */
  getOpenURLs: function () {
    let out = new Set();
    for (let entry of this._store.getAllTabs()) {
      out.add(entry.urlHistory[0]);
    }
    return out;
  },

There's an obvious optimization here, of course: push this into TabStore and don't build the intermediate list.

@@ +90,1 @@
>    locallyOpenTabMatchesURL: function TabEngine_localTabMatches(url) {

This is now only used in these places:


browser/metro/base/content/startui/RemoteTabsView.js
79:        if (tabsEngine.locallyOpenTabMatchesURL(url) || seenURLs.has(url)) {

... fix just like aboutSyncTabs.js

services/sync/tests/unit/test_tab_engine.js
33:  _("test locallyOpenTabMatchesURL");
42:  matches = engine.locallyOpenTabMatchesURL("http://foo.com");
46:  matches = engine.locallyOpenTabMatchesURL("http://barfoo.com");

... replace with a membership check to test getOpenURLs.


Then kill locallyOpenTabMatchesURL.

Comment 3

[Richard Newman [:rnewman]]

Note to the original reporter: on Fennec, Sync runs in the background; loading the synced tabs list is as simple as a DB query. On Desktop, Firefox actually does a little tab sync as you load that page.

Comment 4

[Valery Yundin]

Attachment: Speed up about:sync-tabs loading time - r2

A revised patch with proposed changes.

Comment 5

[Valery Yundin]

Attachment: Avoid expensive getBrowserState call in getAllTabs

This is an optional addition to the previous patch.

It avoids building full browser state (and converting it to JSON and back), which contains much more than just the tab data we want. We still need to go through SessionStore functions because only they provide lastAccessed for unloaded tabs.

PS let me mention that contributing to FF dev was quite a disappointing experience. I spent hours trying to figure out how to test these changes in a release version of FF (replacing files in omni.ja doesn't work because of jsloader cache, deleting which in turn breaks something else, etc, etc). It seems to be impossible (or if possible the knowledge is certainly well hidden), so I'm leaving this for someone else to takeover since I have no intention to build a debug version of FF just to change 10 lines of JS code.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Valery Yundin from comment #5)

> hidden), so I'm leaving this for someone else to takeover since I have no
> intention to build a debug version of FF just to change 10 lines of JS code.

Making changes typically involves checking out and building the tree -- the same tree against which you developed your patches. A clean Linux or Mac build should take on the order of fifteen minutes, with incremental builds taking less.

Having that build means you can manually test these changes, but also run all of the unit tests one needs to run to make sure nothing broke.

One typically doesn't attempt to re-package into a different build. As you found, trying to do that would take way longer than it would take to just build Nightly yourself.

If you want to mess around with an existing component at run-time, you can use the Developer Tools. Set `devtools.chrome.enabled` to `true`, then Tools > Web Developer > Inspector.

Inspect about:sync-tabs, click "Console" at the top, and type "RemoteTabViewer" -- you'll see you can mutate the live object in that page, with auto-completion and JS inspection.

That's not a substitute for being able to run test suites, though.

Comment 7

[Valery Yundin]

While building the tree might be relatively simple (assuming that one has all prerequisites installed), it is still puzzles me that there is no way to override a javascript module in a prepackage build. Especially since one can so easily "mess" with it in run-time.

Comment 8

[Richard Newman [:rnewman]]

I had to do a bit of work to Part 2 to make testing possible, and then make the tests work.

   https://hg.mozilla.org/integration/fx-team/rev/aa32872101fe
   https://hg.mozilla.org/integration/fx-team/rev/6321fef01f69

Thanks again for the patches!

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa32872101fe
https://hg.mozilla.org/mozilla-central/rev/6321fef01f69

Comment 10

[Valery Yundin]

Hi, thanks for adding the tests. There is one thing which caught my eye, is this "dump" statement intentional or just debugging leftover https://hg.mozilla.org/mozilla-central/rev/6321fef01f69#l1.43 ?

Comment 11

[Richard Newman [:rnewman]]

Good catch, Valery, thanks!

https://hg.mozilla.org/integration/fx-team/rev/7455653685a1

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7455653685a1

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8395396 [details] [diff] [review]
Speed up about:sync-tabs loading time - r2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Forever.

User impact if declined: 
  about:sync-tabs is slow; this dramatically speeds it up with little risk. Not requesting uplift for Part 2, which has more risk.

Testing completed (on m-c, etc.): 
  Tested locally, baked on Nightly.

Risk to taking this patch (and alternatives if risky): 
  Low risk: it basically caches a value rather than computing it once per incoming tab.

String or IDL/UUID changes made by this patch:
  None.

Comment 14

[Richard Newman [:rnewman]]

Thanks for the quick approval, Lukas!

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac8cccaa72b5