1382170 - New Preferences is too slow

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 3

[Florian Quèze [:florian]]

(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.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 :'( ...)

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(The emoji issue you had is tracked as https://bugzilla.mozilla.org/show_bug.cgi?id=1253535)

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1382170 - Only run gSearchResultsPane.initializeCategories() when search is used for the first time,

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,
the function is run already when the user starts search.

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

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d10315bb9138
Only run gSearchResultsPane.initializeCategories() when search is used for the first time, r=jaws

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/d10315bb9138