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

RESOLVED FIXED in Firefox 30

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: samth, Assigned: yuvaleriy+persona)

Tracking

unspecified
mozilla31
Points:
---

Firefox Tracking Flags

(firefox29 wontfix, firefox30 fixed, firefox31 fixed)

Details

(Whiteboard: [only part 1 uplifted, intentionally])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Component: General → Firefox Sync: UI
Product: Firefox → Mozilla Services
Version: 21 Branch → unspecified
(Assignee)

Comment 1

5 years ago
Created attachment 8394922 [details] [diff] [review]
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 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.
Attachment #8394922 - Flags: feedback+
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.
Assignee: nobody → yuvaleriy+persona
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 4

5 years ago
Created attachment 8395396 [details] [diff] [review]
Speed up about:sync-tabs loading time - r2

A revised patch with proposed changes.
Attachment #8394922 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 8395402 [details] [diff] [review]
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.
(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.
Attachment #8395396 - Flags: review?(rnewman)
Attachment #8395402 - Flags: review?(rnewman)
(Assignee)

Comment 7

5 years ago
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.
Attachment #8395396 - Flags: review?(rnewman) → review+
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!
https://hg.mozilla.org/mozilla-central/rev/aa32872101fe
https://hg.mozilla.org/mozilla-central/rev/6321fef01f69
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Assignee)

Comment 10

5 years ago
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 ?
Attachment #8395402 - Flags: review?(rnewman) → review+
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.
Attachment #8395396 - Flags: approval-mozilla-aurora?
Attachment #8395396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks for the quick approval, Lukas!

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac8cccaa72b5
status-firefox29: --- → wontfix
status-firefox30: --- → fixed
status-firefox31: --- → fixed
Whiteboard: [only part 1 uplifted, intentionally]
You need to log in before you can comment on or make changes to this bug.