New Preferences is too slow

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Preferences
P3
normal
RESOLVED FIXED
a month ago
5 days ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

(Depends on: 2 bugs)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

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

Attachments

(1 attachment)

According to Gecko Profiler https://perfht.ml/2u97JC4 , the page now takes 457ms just to run init_all()...

I am try to lazily load what I could find to get this back to a better state.
Over in bug 1379338, I'm seeing significant gains in overall prefs load perf.  I'll post a new patch today, based on my webify-prefs branch <https://github.com/mykmelez/gecko/tree/webify-prefs>.  I'm curious if those changes improve perf in your profile as well.
See Also: → bug 1379338
Given that there is already a patch there, I am going to work on more preformance tweaks with the WIP patch applied, so we would know what else to speed up.
Depends on: 1379338
See Also: bug 1379338
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #0)
> According to Gecko Profiler https://perfht.ml/2u97JC4 , the page now takes
> 457ms just to run init_all()...

Most of the time in your profile is bug 1375870, that should be easy to fix without blocking on Myk's refactoring.

The other slow thing in your profile is buildFontList, see bug 1375978.
Depends on: 1375870, 1375978

Updated

28 days ago
Whiteboard: [photon-onboarding] → [photon-preference]
I believe what made Preferences so slow is that we are now calling init_all during load. It wasn't supposed to be that way. 

Initially, init_all was only called when the search box was focused so we could make sure that all categories could be searched. But now since bug 1374852 we focus the search box on load which causes all categories to get loaded initially.

We need to either back out bug 1374852 or find another time to load all search panes.
Flags: needinfo?(timdream)
Flags: needinfo?(timdream)
Thanks for the information. I didn't look into that patch. Thank you for point that to me as a reviewer.

Instead of back that out, I think I can shuffle the things it does a bit to make it look faster. I am going to work on bug 1375978 and come back to this one later.

(Bugzilla filters out Emoji so my previous comment appears empty :'( ...)
(The emoji issue you had is tracked as https://bugzilla.mozilla.org/show_bug.cgi?id=1253535)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> (The emoji issue you had is tracked as
> https://bugzilla.mozilla.org/show_bug.cgi?id=1253535)

Yeah I thought it was fixed already. :'(
Comment hidden (mozreview-request)
The patch I pushed here does comment 6. I wasn't able to do another profile but from the profile in comment 0 this should save us 250ms.
See Also: → bug 1384830

Comment 11

21 days ago
mozreview-review
Comment on attachment 8890696 [details]
Bug 1382170 - Only run gSearchResultsPane.initializeCategories() when search is used for the first time,

https://reviewboard.mozilla.org/r/161882/#review167494

::: commit-message-f1693:6
(Diff revision 1)
> +Bug 1382170 - Only run gSearchResultsPane.initializeCategories() when search is used for the first time, r=jaws
> +
> +This shuffled what bug 1374852 did a little bit; instead of initialize all panes
> +when the page loads, the call will happen inside an idle callback.
> +
> +An the event listener is added to ensure that even if idel callback never runs,

s/idel/idle/

and not that it "never runs", but maybe "doesn't run soon enough"

::: browser/components/preferences/in-content-new/findInPage.js:21
(Diff revision 1)
>      this.inited = true;
>      this.searchInput = document.getElementById("searchInput");
>      this.searchInput.hidden = !Services.prefs.getBoolPref("browser.preferences.search");
>      if (!this.searchInput.hidden) {
> -      this.searchInput.addEventListener("command", this);
>        window.addEventListener("load", () => {

Can we change this to domcontentloaded?

::: browser/components/preferences/in-content-new/findInPage.js:26
(Diff revision 1)
> +      this.searchInput.addEventListener("command", event => this.initializeCategories(), { once: true });
> +      this.searchInput.addEventListener("command", this);

Are we sure that the first event listener listed here will be executed before the second one? I don't know what the spec says?

Either way, what about just calling initializeCategories from searchFunction()? The function will early-return if it has already been called.
Attachment #8890696 - Flags: review?(jaws) → review+
(Assignee)

Comment 12

21 days ago
mozreview-review
Comment on attachment 8890696 [details]
Bug 1382170 - Only run gSearchResultsPane.initializeCategories() when search is used for the first time,

https://reviewboard.mozilla.org/r/161882/#review167640

::: browser/components/preferences/in-content-new/findInPage.js:26
(Diff revision 1)
> +      this.searchInput.addEventListener("command", event => this.initializeCategories(), { once: true });
> +      this.searchInput.addEventListener("command", this);

The spec says the event listener is appended.

https://dom.spec.whatwg.org/#dom-eventtarget-addeventlistener

But yeah you are right, we should not be rely on DOM impl for this.
Comment hidden (mozreview-request)
Keywords: checkin-needed

Comment 14

18 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d10315bb9138
Only run gSearchResultsPane.initializeCategories() when search is used for the first time, r=jaws
Keywords: checkin-needed

Comment 15

17 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d10315bb9138
Status: ASSIGNED → RESOLVED
Last Resolved: 17 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Depends on: 1389550
No longer depends on: 1379338
You need to log in before you can comment on or make changes to this bug.