1369539 - FxAccounts modules loaded too early at startup

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, enhancement, P2)

Description

[Lina Butler [:lina]]

Splitting out from bug 1369436:

> Slightly off-topic for this bug, but there are currently 4 fx account
> modules loaded after first paint but before we handle user events:
> FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm
> FxAccountsStorage.jsm 
> Could these also be loaded lazily?

Comment 1

[Florian Quèze [:florian]]

Here is a startup profile showing it: https://perfht.ml/2r7dyLE

This is started from _delayedStartup in browser.js (so I guess http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/browser/base/content/browser.js#1639)

Can we skip most of this when sync has never been configured (no username)?

Comment 2

[Florian Quèze [:florian]]

Also visible in profiles of creating new windows: http://bit.ly/2rzrLBY (this is on a slow netbook)

I doubt this.EnsureFxAccountsWebChannel is needed for each new window; should some of this be moved from browser.js _delayedStartup to nsBrowserGlue.js?

Comment 3

[Edouard Oger [:eoger]]

Florian would moving the gSync.init() call [0] later in _delayedStartup (or even wrapping it in a requestIdleCallback) work for you?

[0] http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1638

Comment 4

[Florian Quèze [:florian]]

(In reply to Edouard Oger [:eoger] from comment #3)
> Florian would moving the gSync.init() call [0] later in _delayedStartup (or
> even wrapping it in a requestIdleCallback) work for you?
> 
> [0]
> http://searchfox.org/mozilla-central/source/browser/base/content/browser.
> js#1638

It's already in _delayedStartup, so moving it later in _delayedStartup wouldn't improve anything for performance (all of that function will run before we return to the event loop). Calling it from an idle callback would certainly be an improvement.

I still think we should completely skip this when we have no sync username in the preferences.

Comment 5

[Edouard Oger [:eoger]]

Attachment: Bug 1369539 - Sync UI startup performance improvements.

Review commit: https://reviewboard.mozilla.org/r/154256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154256/

Comment 6

[Edouard Oger [:eoger]]

5 sec max timeout okay for you Mark?

Comment 7

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> I still think we should completely skip this when we have no sync username
> in the preferences.

So is there a reason why this isn't possible?

I think you'll want to blacklist FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm and FxAccountsStorage.jsm in http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/browser/base/content/test/performance/browser_startup.js#93

Comment 8

[Edouard Oger [:eoger]]

Yes, EnsureFxAccountsWebChannel is needed even when sync is un-configured at the moment. We would need to refactor it if we wanted to make this lazy.
In any case we still require FxAccounts.jsm in UIState.jsm anyway.

Comment 9

[Edouard Oger [:eoger]]

> I think you'll want to blacklist FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm and FxAccountsStorage.jsm in http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/browser/base/content/test/performance/browser_startup.js#93

This makes the test fail :/
Commenting |gSync.init()| makes the test pass, so we can assume no one else is initializing these modules.

Comment 10

[Florian Quèze [:florian]]

(In reply to Edouard Oger [:eoger] from comment #9)
> > I think you'll want to blacklist FxAccountsCommon.js FxAccountsWebChannel.jsm FxAccounts.jsm and FxAccountsStorage.jsm in http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/browser/base/content/test/performance/browser_startup.js#93
> 
> This makes the test fail :/
> Commenting |gSync.init()| makes the test pass, so we can assume no one else
> is initializing these modules.

That's likely because the test also uses requestIdleCallback, and these 2 calls compete. I guess then we can't test this until loading all of this becomes lazy.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8883367 [details]
Bug 1369539 - Sync UI startup performance improvements.

https://reviewboard.mozilla.org/r/154256/#review159362

r+ because this is a net improvement over the current situation, but I think we can (and should) do better.

(In reply to Edouard Oger [:eoger] from comment #8)
> Yes, EnsureFxAccountsWebChannel is needed even when sync is un-configured at
> the moment.

Needed by what for which purpose?

> We would need to refactor it if we wanted to make this lazy.

Seems like a reasonable thing to do ;-).

> In any case we still require FxAccounts.jsm in UIState.jsm anyway.

So... can we ensure none of the sync code ever gets loaded unless either sync is already configured, or the user touches the sync configuration UI? Even the browser-sync.js file probably wants to become lazy loaded.

Comment 12

[Edouard Oger [:eoger]]

Comment on attachment 8883367 [details]
Bug 1369539 - Sync UI startup performance improvements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154256/diff/1-2/

Comment 13

[Edouard Oger [:eoger]]

How about this? This avoids loading FxAccounts.jsm in UIState.jsm if no username is defined.

> (In reply to Edouard Oger [:eoger] from comment #8)
> > Yes, EnsureFxAccountsWebChannel is needed even when sync is un-configured at
> > the moment.
> 
> Needed by what for which purpose?

Currently we have no way to open a WebChannel instance until a web page actually needs it. (See bug 1358326)

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8883367 [details]
Bug 1369539 - Sync UI startup performance improvements.

https://reviewboard.mozilla.org/r/154256/#review159392

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #6)
> 5 sec max timeout okay for you Mark?

Sounds fine given it is supposed to be the max time, thanks.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dec7cb09336e
Sync UI startup performance improvements. r=florian,markh

Comment 17

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

Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=113149493&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/ab47a6ff213af129cb4e78828c8b02c23fb9f784

Comment 18

[Edouard Oger [:eoger]]

Comment on attachment 8883367 [details]
Bug 1369539 - Sync UI startup performance improvements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154256/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89fda6fb1946
Sync UI startup performance improvements. r=florian,markh

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89fda6fb1946

Comment 21

[Florian Quèze [:florian]]

The changes made to browser.js here covered the normal browser window case, but didn't cover the gBrowserInit.nonBrowserWindowDelayedStartup case; I'm applying the same change there in bug 1381853 attachment 8888419 [details] [diff] [review].