Closed Bug 988051 Opened 8 years ago Closed 8 years ago
Lazily load Fx
Accounts Client .jsm
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
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.
eg, this patch avoids ~12 modules loading
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 on attachment 8396840 [details] [diff] [review] Lazy FxAccounts How about this one then? :) I'll update the bug title accordingly if this gets review.
Attachment #8396840 - Flags: review?(rnewman)
Attachment #8396840 - Flags: review?(rnewman) → review+
Attachment #8396769 - Attachment is obsolete: true
awesome, thanks! https://hg.mozilla.org/integration/fx-team/rev/26719e92872b
Assignee: MattN+bmo → mhammond
Summary: Lazily load hawkrequest.js and Credentials.jsm → Lazily load FxAccountsClient.jsm
If this sticks (it should; xpcshell tests all worked locally!) then we should probably uplift it.
> this patch avoids ~12 modules loading \o/
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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 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?
(In reply to :Gavin Sharp (email firstname.lastname@example.org) 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.
You need to log in before you can comment on or make changes to this bug.