Closed Bug 1382170 Opened 3 years ago Closed 3 years ago

New Preferences is too slow

Categories

(Firefox :: Preferences, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

(Depends on 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

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.
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: 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
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 :'( ...)
(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. :'(
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 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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/d10315bb9138
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Depends on: 1389550
No longer depends on: 1379338
Flags: qe-verify-
Depends on: 1425398
You need to log in before you can comment on or make changes to this bug.