Closed Bug 988051 Opened 8 years ago Closed 8 years ago

Lazily load FxAccountsClient.jsm

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 + fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: MattN, Assigned: markh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Memshrink])

Attachments

(1 file, 1 obsolete file)

Attached patch v.1 defineLazyModuleGetter (obsolete) — Splinter Review
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
Attachment #8396769 - Flags: review?(rnewman)
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.
Attached patch Lazy FxAccountsSplinter Review
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.
Attachment #8396769 - Flags: review?(rnewman)
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.
Thanks Mark!
> this patch avoids ~12 modules loading

\o/
https://hg.mozilla.org/mozilla-central/rev/26719e92872b
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
Attachment #8396840 - Flags: approval-mozilla-beta?
Attachment #8396840 - Flags: approval-mozilla-aurora?
Attachment #8396840 - Flags: approval-mozilla-beta?
Attachment #8396840 - Flags: approval-mozilla-beta+
Attachment #8396840 - Flags: approval-mozilla-aurora?
Attachment #8396840 - Flags: approval-mozilla-aurora+
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 gavin@gavinsharp.com) 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.
Product: Core → Firefox
Target Milestone: mozilla31 → Firefox 31
You need to log in before you can comment on or make changes to this bug.