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)

Product:
Firefox
Component:
Settings UI
Platform:
Unspecified
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

Details

(Keywords: perf, Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1400117 - Initialize Application handlers after pageshow to avoid delaying painting of Preferences page
59 bytes, text/x-review-board-request
jaws
: review+
florian
: review+
Details 
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
Blocks: 1398597
No longer depends on: 1398597
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+
Flags: qe-verify+
Whiteboard: [photon-preference][triage] → [photon-preference]
No longer blocks: photon-preference
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.
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
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.
Thanks, update the patch for listening "pageshow" event.
(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.)
(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?
(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!
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 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+
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
Summary: Introduce requestAnimationFrame to initialize panel asynchronously → Initialize Application handlers after pageshow to avoid delaying painting of Preferences page
https://hg.mozilla.org/mozilla-central/rev/f4714012ceb4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
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)
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
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.