UnifiedComplete::LoadPrefs is slow

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
2 months ago
16 days ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug, {perf})

55 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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
and we could also investigate lazifying most of the prefs.
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.
(Assignee)

Updated

2 months ago
Keywords: perf
Priority: P2 → P1
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8890883 - Flags: review?(paolo.mozmail)
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

18 days ago
mozreview-review
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.
Attachment #8890883 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)

Comment 8

17 days ago
mozreview-review
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.
Attachment #8890883 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)

Comment 10

17 days ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/78b83c499b5d
UnifiedComplete::LoadPrefs is slow. r=Paolo

Comment 11

16 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/78b83c499b5d
Status: ASSIGNED → RESOLVED
Last Resolved: 16 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.