988051 - Lazily load FxAccountsClient.jsm

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Matthew N. [:MattN]]

Attachment: v.1 defineLazyModuleGetter

I verified these two modules aren't loaded on startup in a clean profile after the change.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e068de02cb0b

Comment 1

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

It seems this might be attacking the wrong end of the problem - eg, we should only be loading hawkclient once we know it's going to be used - at which point deferring the import of hawkrequest probably doesn't buy anything as it would immediately be used.  IOW, we probably want to defer hawkclient itself in the modules that use it.

I think a bigger bang-for-buck would be to look at FxAccounts.jsm - many of the modules there are only used if a user is logged in, which will not be true for a clean profile.  Once lazy imports there are loaded, I'm not sure there's huge benefit in subsequent lazy-loads as they are almost certainly going to be used relatively quickly after import.

Comment 2

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

Attachment: Lazy FxAccounts

eg, this patch avoids ~12 modules loading

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8396769 [details] [diff] [review]
v.1 defineLazyModuleGetter

Yeah, I'm in favor of moving the laziness up to the decision point of the functionality, rather than swizzling it in throughout.

Any user of hawkclient.js will very shortly use hawkrequest.js, so we're not really saving anything.

Comment 4

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

Comment on attachment 8396840 [details] [diff] [review]
Lazy FxAccounts

How about this one then? :)  I'll update the bug title accordingly if this gets review.

Comment 5

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

awesome, thanks!

https://hg.mozilla.org/integration/fx-team/rev/26719e92872b

Comment 6

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

If this sticks (it should; xpcshell tests all worked locally!) then we should probably uplift it.

Comment 7

[Matthew N. [:MattN]]

Thanks Mark!

Comment 8

[Nicholas Nethercote [inactive]]

> this patch avoids ~12 modules loading

\o/

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26719e92872b

Comment 10

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

Comment on attachment 8396840 [details] [diff] [review]
Lazy FxAccounts

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA Sync
User impact if declined: Higher Fx memory usage when no FxA user logged in
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8396840 [details] [diff] [review]
Lazy FxAccounts

>diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm

> Cu.import("resource://gre/modules/Timer.jsm");
> Cu.import("resource://gre/modules/FxAccountsCommon.js");
> Cu.import("resource://gre/modules/FxAccountsUtils.jsm");

Shouldn't some of these be lazy-loaded too? Is there another bug for that?

Comment 12

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

(In reply to :Gavin Sharp (email [e-mail]) from comment #11)
> Comment on attachment 8396840 [details] [diff] [review]
> Lazy FxAccounts
> 
> >diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm
> 
> > Cu.import("resource://gre/modules/Timer.jsm");
> > Cu.import("resource://gre/modules/FxAccountsCommon.js");
> > Cu.import("resource://gre/modules/FxAccountsUtils.jsm");
> 
> Shouldn't some of these be lazy-loaded too? Is there another bug for that?

Personally I'm not bothered by them.  FxAccountsUtils will be immediately be used (and should die IMO anyway), FxAccountsCommon ideally would, but it's imported without a "namespace" so this change would be far more intrusive, and I see Timer.jsm like Log.jsm - a general utility module that's likely to be already loaded.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d46c23f82bfa
https://hg.mozilla.org/releases/mozilla-beta/rev/0fc15cf33a3c