Closed Bug 1426627 Opened 2 years ago Closed 2 years ago

Typing in the urlbar loads 3 sync modules even when sync isn't enabled

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: florian, Assigned: tcsc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

While looking at a profile of the browser/base/content/test/performance/browser_urlbar_search_reflows.js test, I noticed that these 3 JS modules are loaded as soon as the user starts typing in the urlbar:

resource://services-sync/constants.js
resource://services-sync/main.js
resource://gre/modules/PlacesRemoteTabsAutocompleteProvider.jsm

It's not very expensive in the profile I saw (was captured on fast machine), but seems wasteful.

Is there a cheap test we could put around https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/toolkit/components/places/UnifiedComplete.js#1683 to avoid triggering the PlacesRemoteTabsAutocompleteProvider lazy getter when sync isn't enabled?
Services.prefs.prefHasUserValue("services.sync.username") is the usual test for 'are they a sync user'.
Assignee: nobody → tchiovoloni
The usual test (e.g. a pref check) will still end up with a preference checks, which seems worse than the module loading (at least, it would undo the work from bug 1427850), so there are two options: Either do roughly what we do in bug 1427850, or just directly use a lazy preference getter. This does the 1st, but I can switch to the 2nd if you think that's cleaner.

LMK if you think I should get someone from the sync team to review instead.
Priority: -- → P1
I think I would prefer a lazy pref getter. Cc'ing Gijs who cares about how many times we read prefs and may have an opinion.
See Also: → 1427850
(In reply to Florian Quèze [:florian] from comment #4)
> I think I would prefer a lazy pref getter. Cc'ing Gijs who cares about how
> many times we read prefs and may have an opinion.

The lazy pref getter will read the pref only once (or if/when it changes, which is somewhat unlikely). That's likely going to be fastest, certainly after the first get (so any subsequent keypresses).

The weave stuff will fetch an xpcom service. I don't know off-hand if that's initialized elsewhere already. (Perhaps it should have a Services alias if many components need it... (x-ref bug 1425617)). I don't know how much the weave service does when initialized if that does happen here.

If we know for sure the weave service will already be initialized at this point (and accessed from JS at least once), and we expect that to be the case in the foreseeable future, too*** (and/or initialization is cheap) then that would be slightly preferable over the pref read because the pref read is essentially making UnifiedComplete depend on sync internals (ie where it stores the username), which seems unfortunate. In all other cases, I would prefer a lazy pref getter.


*** if it's expensive and we want to move it out of the startup path, then adding dependencies on it having started up in said path is not a good idea, of course.
> The weave stuff will fetch an xpcom service. I don't know off-hand if that's initialized elsewhere already. (Perhaps it should have a Services alias if many components need it... (x-ref bug 1425617)).

The main uses of the service are checking if sync is enabled, and checking if sync has finished loading (well, and getting a promise that resolves when it's loaded). In practice, it's just used for the 2nd, since most code just reads the preference [0].

> the pref read is essentially making UnifiedComplete depend on sync internals (ie where it stores the username), which seems unfortunate

There are already enough users of services.sync.username [0] that it's basically stable even though it's kind of a cludge.  Even if we move to a different method of storing internal state (bug 1426480), this would still be stored in preferences since, well, it's a user configurable preference, and it changes only very rarely.

(Although, if the weave service were on Services, all these would probably be able to move to using that instead, which would be cleaner)

> If we know for sure the weave service will already be initialized at this point (and accessed from JS at least once), and we expect that to be the case in the foreseeable future, too*** (and/or initialization is cheap)

It should be initialized for sync users [1], but I think other users won't have accessed it yet.

I think it's initialization is cheap. The module [2] is 165 lines, and unless you actually call methods on it, it doesn't do a lot other than some declarations and such. That said, AFAIK it does require loading that component, which might count as expensive, I don't know (naively ISTM like it would require synchronous IO to load the component on demand, which sounds expensive, but it's possible I'm wrong).

> *** if it's expensive and we want to move it out of the startup path, then adding dependencies on it having started up in said path is not a good idea, of course.

The bulk of the sync initialization happens outside of the startup path, which is part of why the weave service is not particularly useful aside from checking if sync is ready and/or enabled.

---

Anyway, whether or not the pref read is cleaner than `!weaveService || !weaveService.ready || !weaveService.enabled` is certainly not 100% clear to me, both seem kind of gross. (Admittedly, that's especially cludgey due to Thunderbird and such, and the `.ready` check is not strictly necessary, it just avoids the lazy pref read for when sync hasn't started up yet, and probably could/should be omitted).

The cleanest solution would probably be to put it on Services, but I don't know if it meets the usefulness requirements for that, and definitely doesn't seem like the kind of thing to do in this bug.

Honestly I'm not sure where this leaves us though, I don't feel very strongly either way.

[0]: https://searchfox.org/mozilla-central/search?q=services.sync.username
[1]: https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#957
[2]: https://searchfox.org/mozilla-central/source/services/sync/Weave.js
Comment on attachment 8940863 [details]
Bug 1426627 - Avoid loading sync modules for non-sync users in UnifiedComplete

https://reviewboard.mozilla.org/r/211110/#review217224

(In reply to Thom Chiovoloni [:tcsc] from comment #6)

> Honestly I'm not sure where this leaves us though, I don't feel very
> strongly either way.

Let's go with a lazy pref getter.

::: toolkit/components/places/UnifiedComplete.js:335
(Diff revision 1)
>                                     "@mozilla.org/intl/texttosuburi;1",
>                                     "nsITextToSubURI");
>  
> +XPCOMUtils.defineLazyGetter(this, "weaveService", function() {
> +  return Cc["@mozilla.org/weave/service;1"]
> +           .getService(Ci.nsISupports)

nit for future patches: .getService(Ci.nsISupports) can be simplified to .getService()
Attachment #8940863 - Flags: review?(florian)
(In reply to Thom Chiovoloni [:tcsc] from comment #6)

> I think it's initialization is cheap. The module [2] is 165 lines, and
> unless you actually call methods on it, it doesn't do a lot other than some
> declarations and such.

You are right that it doesn't seem to do much and would probably be cheap. My concern about loading the component are that:
- it would stay in memory but provide no user value for users without sync configured
- it could potentially start doing more things in the future and be less cheap than it currently is. To avoid this I would want to put tests verifying that this doesn't import another few files that in turn import some more, ... Let's not worry about this now :-).

> That said, AFAIK it does require loading that
> component, which might count as expensive, I don't know (naively ISTM like
> it would require synchronous IO to load the component on demand, which
> sounds expensive, but it's possible I'm wrong).

This would sometimes do main thread IO, indeed. But that's arguably a bug of our current omni.ja cache.

> The cleanest solution would probably be to put it on Services, but I don't
> know if it meets the usefulness requirements for that, and definitely
> doesn't seem like the kind of thing to do in this bug.

Adding this on Services wouldn't avoid the cost of loading it the first time we access something from it.
Comment on attachment 8940863 [details]
Bug 1426627 - Avoid loading sync modules for non-sync users in UnifiedComplete

https://reviewboard.mozilla.org/r/211110/#review217236

Looks good to me, thanks!
Attachment #8940863 - Flags: review?(florian) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b41371e4b3e
Avoid loading sync modules for non-sync users in UnifiedComplete r=florian
https://hg.mozilla.org/mozilla-central/rev/7b41371e4b3e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.