1371611 - UnifiedComplete::LoadPrefs is slow

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Marco Bonardo [:mak]]

Florian showed me this profile where LoadPrefs is taking 55ms
https://perfht.ml/2r1TR92

Since this may happen early on startup, it should be faster.

Our suspects are:
- Preferences.jsm instead of Services.prefs
- wide usage of the spread operator

Comment 1

[Marco Bonardo [:mak]]

and we could also investigate lazifying most of the prefs.

Comment 2

[(no longer active)]

FWIW I'd reprofile this a bunch of times before spending a lot of time on it.  Preferences.get() certainly shows up here, but so does InitStringClass()!  So be careful not to read too much into one time measurements like this.

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1371611 - UnifiedComplete::LoadPrefs is slow.

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

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8890883 [details]
Bug 1371611 - UnifiedComplete::LoadPrefs is slow.

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

Comment 5

[Marco Bonardo [:mak]]

The patch removes the obvious causes of slowness:
1. a .reduce call, replaced by a simple loop
2. Preferences.jsm calls
3. Loading all the prefs on startup

Now most of the prefs are read lazily on demand.

There are a few edge cases that for now are not yet nicely handled, but we will be able to handle those once we move to 57, in future bugs:
1. the link between autocomplete.enabled and suggest.* prefs. Once we drop add-ons compat, we can likely just rely on the suggest.* prefs and remove the old autocomplete.enabled. This way we lose syncing of that pref across devices, but it's likely not critical and different devices may have different usage.
2. the .typed prefs (suggest.history.onlyTyped and autoFill.typed) can probably be removed as well, typed is broken and the likely for the user to know he is looking for a "typed" url is very low. autoFill.typed may just be the default until we revise the autoFill algo.

Comment 6

[:Paolo Amadini]

Comment on attachment 8890883 [details]
Bug 1371611 - UnifiedComplete::LoadPrefs is slow.

https://reviewboard.mozilla.org/r/162096/#review168734

::: toolkit/components/places/UnifiedComplete.js:22
(Diff revision 2)
> -const MATCH_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE;
> -const MATCH_BOUNDARY_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY_ANYWHERE;
> -const MATCH_BOUNDARY = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY;
> -const MATCH_BEGINNING = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING;
> -const MATCH_BEGINNING_CASE_SENSITIVE = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING_CASE_SENSITIVE;
> -
> +const { MATCH_ANYWHERE,
> +        MATCH_BOUNDARY_ANYWHERE,
> +        MATCH_BOUNDARY,
> +        MATCH_BEGINNING,
> +        MATCH_BEGINNING_CASE_SENSITIVE,
> +      } = Ci.mozIPlacesAutoComplete;

style nit:

const {
  ...,
  ...,
} = ...;

::: toolkit/components/places/UnifiedComplete.js:26
(Diff revision 2)
> -const PREF_AUTOFILL =               [ "autoFill",               true ];
> -const PREF_AUTOFILL_TYPED =         [ "autoFill.typed",         true ];
> -const PREF_AUTOFILL_SEARCHENGINES = [ "autoFill.searchEngines", false ];
> +const PREF_URLBAR_DEFAULTS = new Map([
> +  [ "autocomplete.enabled",             true                                  ],
> +  [ "autoFill",                         true                                  ],

style nit: I'm not a fan of the tabular format. You can keep it, but I'd prefer something like this (with no space after opening bracket):

const PREF_URLBAR_DEFAULTS = new Map([
  ["autocomplete.enabled", true],
  ...,
  ["matchBucketsSearch", ""],
]);

::: toolkit/components/places/UnifiedComplete.js:446
(Diff revision 2)
>  XPCOMUtils.defineLazyGetter(this, "Prefs", () => {
> -  let prefs = new Preferences(PREF_BRANCH);
> -  let types = ["History", "Bookmark", "Openpage", "Searches"];
> -
> -  function syncEnabledPref() {
> -    loadSyncedPrefs();
> +  let branch = Services.prefs.getBranch(PREF_URLBAR_BRANCH);
> +  let types = ["history", "bookmark", "openpage", "searches"];
> +  let prefTypes = new Map([["boolean", "Bool"], ["string", "Char"], ["number", "Int"]]);
> +
> +  let readPref = pref => {

Since you're not using "this", it's probably clearer to use function statements.

::: toolkit/components/places/UnifiedComplete.js:459
(Diff revision 2)
> -        prefs.set(PREF_ENABLED[0], types.some(type => store["suggest" + type]));
> -      }
> -    }
>  
> -    store.enabled = prefs.get(...PREF_ENABLED);
> -    store.autofill = prefs.get(...PREF_AUTOFILL);
> +  let getPrefValue = pref => {
> +    if (pref == "matchBuckets") {

Sounds like you want a switch statement? You can still scope blocks with {}.

::: toolkit/components/places/UnifiedComplete.js:533
(Diff revision 2)
> +  let linkingPrefs = false;
> +  let updateLinkedPrefs = (changedPref = "") => {
> +    // Avoid re-entrance.
> +    if (linkingPrefs)
> +      return;
> +    linkingPrefs = true;

Add a try-finally for good measure.

::: toolkit/components/places/UnifiedComplete.js:581
(Diff revision 2)
> -  // Synchronize suggest.* prefs with autocomplete.enabled at initialization
> -  syncEnabledPref();
> -
> -  loadPrefs();
> +  let store = new Proxy(new Map(), {
> +    get: (target, name) => {
> +      switch (name) {
> +        case "get":

I don't think you need a Proxy since you're using only the methods you're defining here. Define a new Map as the store separately, and define an object with methods to access it.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8890883 [details]
Bug 1371611 - UnifiedComplete::LoadPrefs is slow.

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

Comment 8

[:Paolo Amadini]

Comment on attachment 8890883 [details]
Bug 1371611 - UnifiedComplete::LoadPrefs is slow.

https://reviewboard.mozilla.org/r/162096/#review168876

Looks much better, thanks!

::: toolkit/components/places/UnifiedComplete.js:532
(Diff revisions 2 - 3)
>  
>    // Used to keep some pref values linked.
>    // TODO: remove autocomplete.enabled and rely only on suggest.* prefs once we
>    // can drop legacy add-ons compatibility.
>    let linkingPrefs = false;
> -  let updateLinkedPrefs = (changedPref = "") => {
> +  let updateLinkedPrefs = function(changedPref = "") {

nit: I was thinking of just...

function updateLinkedPrefs(changedPref = "")

...which should work fine in strict mode too as long as it is not inside a conditional block, but the current version is already better, and it's just as readable.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8890883 [details]
Bug 1371611 - UnifiedComplete::LoadPrefs is slow.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78b83c499b5d
UnifiedComplete::LoadPrefs is slow. r=Paolo

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/78b83c499b5d