Closed
Bug 1400117
Opened 6 years ago
Closed 6 years ago
Initialize Application handlers after pageshow to avoid delaying painting of Preferences page
Categories
(Firefox :: Settings UI, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: rickychien, Assigned: rickychien)
References
Details
(Keywords: perf, Whiteboard: [photon-preference])
Attachments
(1 file)
This is way too slow to load Preferences on Windows at the first time. We're going to introduce a front-end workaround and replace setTimeout with requestAnimationFrame to see whether it can reduce the initialization time. This approach is suggested by https://bugzilla.mozilla.org/show_bug.cgi?id=1398597#c14, the implementation might look like https://bugzilla.mozilla.org/show_bug.cgi?id=1398597#c15
Assignee | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8908472 [details] Bug 1400117 - Initialize Application handlers after pageshow to avoid delaying painting of Preferences page https://reviewboard.mozilla.org/r/180122/#review185270 How are you verifying that this fixes the issue? Do you have before/after numbers already calculated? ::: commit-message-8e818:1 (Diff revision 1) > +Bug 1400117 - Introduce requestAnimationFrame to initialize panel asynchronously r?jaws Can you please change the commit message to explain why this should be done asynchronously? ::: browser/components/preferences/in-content/main.js:439 (Diff revision 1) > } else { > this._sortColumn = document.getElementById("typeColumn"); > } > > // Load the data and build the list of handlers. > // By doing this in a timeout, we let the preferences dialog resize itself This comment should be updated to explain the requestAnimationFrame call. ::: browser/components/preferences/in-content/main.js:444 (Diff revision 1) > // By doing this in a timeout, we let the preferences dialog resize itself > // to an appropriate size before we add a bunch of items to the list. > // Otherwise, if there are many items, and the Applications prefpane > // is the one that gets displayed when the user first opens the dialog, > // the dialog might stretch too much in an attempt to fit them all in. > // XXX Shouldn't we perhaps just set a max-height on the richlistbox? Did you try setting a max-height on the richlistbox?
Attachment #8908472 -
Flags: review?(jaws) → review+
Updated•6 years ago
|
Flags: qe-verify+
Whiteboard: [photon-preference][triage] → [photon-preference]
Assignee | ||
Updated•6 years ago
|
No longer blocks: photon-preference
Assignee | ||
Comment 3•6 years ago
|
||
I don't see any visible perf improvement after applying this patch while testing on a slow Windows laptop. Reference Device (Windows Laptop): Aspire E 15 Intel i3 7100U (2.4G) 4GB DDR4 Memory Windows 10 It was really slow when launching Nightly at the first time (more than 10s). But it just took 1s for Preferences first opening time without applying this patch.
Assignee | ||
Comment 4•6 years ago
|
||
Florian, Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1398597#c27. I'm not clear why `requestAnimationFrame + setTimeout` approach can reduce the first loading time of Preferences page. Can you explain the details behind it so I can write down comment for that. I'm not sure this approach can bring a positive impact on performance. At least this will not slow down the page loading time. As a result, I decided to give you a review as well to help ensure the patch is done right. If you think this change will not bring any effect, I'm okay to drop this change and close this issue. Thanks
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
Here are some profiles I got using the try build with the patch: https://perfht.ml/2h9MMTq https://perfht.ml/2hbWx01 https://perfht.ml/2hdceUq Main thread I/O takes a random amount of time, so I haven't reproduced the 4s delay I saw in bug 1398597 comment 14, but 'enumerate' from nsHandlerService-json.js is visible enough in these profiles that we can see the patch isn't having the effect I wanted (in at least 2/3 it's clear that the code runs before first paint, and the third one is less obvious but it clearly runs before the 'pageshow' event). We may get a better result by using a 'pageshow' event listener instead of the requestAnimationFrame call. Note: Bug 1400453 attempts to solve/hide the same problem in a different way; it may be a better approach.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
Thanks, update the patch for listening "pageshow" event.
Comment 10•6 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #7) > Note: Bug 1400453 attempts to solve/hide the same problem in a different > way; it may be a better approach. Even if we take the change in bug 1400453, I think we should take this one as well, since it's explicit about delaying work to populate the Applications list, which reduces the risk of regression from future changes. (It may also gain us some additional perf, if GetProtocolHandlerInfo isn't the only source of slowness.) I would only modify this patch to update the comment above the code being changed, so it no longer says we're delaying handler loading to "let the preferences dialog resize itself" but rather something like: // By doing this in a timeout, we ensure it doesn't delay painting // of the preferences page. (Then I would remove the entire sentence that starts "Otherwise, if there are many items…", since that sentence is also obsolete.)
Comment 11•6 years ago
|
||
(In reply to Myk Melez [:myk] [@mykmelez] from comment #10) > // By doing this in a timeout, we ensure it doesn't delay painting > // of the preferences page. Do we still need the timeout if we are already waiting until the pageshow event?
Comment 12•6 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #11) > (In reply to Myk Melez [:myk] [@mykmelez] from comment #10) > > > // By doing this in a timeout, we ensure it doesn't delay painting > > // of the preferences page. > > Do we still need the timeout if we are already waiting until the pageshow > event? Erm, no, we don't need the timeout, I meant this comment to reference the pageshow event: // By doing this after pageshow, we ensure it doesn't delay painting // of the preferences page. Good catch!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years ago
|
||
Florian, both commit message comment and are updated reflecting Myk's suggestion, setTimeout has been removed since it's useless after replacing with pageshow event. Please review it again. Thanks
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8908472 [details] Bug 1400117 - Initialize Application handlers after pageshow to avoid delaying painting of Preferences page https://reviewboard.mozilla.org/r/180122/#review186498 Looks good (but I haven't tested this locally).
Attachment #8908472 -
Flags: review?(florian) → review+
Comment 16•6 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f4714012ceb4 Initialize Application handlers after pageshow to avoid delaying painting of Preferences page r=florian,jaws
Assignee | ||
Updated•6 years ago
|
Summary: Introduce requestAnimationFrame to initialize panel asynchronously → Initialize Application handlers after pageshow to avoid delaying painting of Preferences page
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f4714012ceb4
Comment 18•6 years ago
|
||
What is the best method that I could use to verify if the first loading time or the initialization time is reduced? Could you please help me with this?
Flags: needinfo?(rchien)
Assignee | ||
Comment 19•6 years ago
|
||
Ah, I should remove qe-verify here as well. This verification should be done in bug 1398597. I've asked reporter to help manual verification at https://bugzilla.mozilla.org/show_bug.cgi?id=1398597#c31. Thanks!
Flags: qe-verify+
Flags: needinfo?(rchien)
QA Contact: hani.yacoub
Updated•6 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•