1400453 - yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

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

Attachment: yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

Here's another potential workaround for bug 1398597.  It should delay calling gExternalProtocolService.getProtocolHandlerInfo (which shows up as expensive in Florian's profile) until _rebuildVisibleTypes and then yield between each call to avoid monopolizing the thread.

This won't reduce the cost of gExternalProtocolService.getProtocolHandlerInfo in profiles, but it should prevent those calls from delaying first paint (and keep the page responsive afterward).

Florian: I don't have Quantum reference hardware, so perhaps you can test this to see what kind of impact it has on time to first paint?

Comment 1

[Florian Quèze [:florian]]

I can't build on the reference hardware, so I pushed your patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21e0ec3cde516b2e56c9ceda9192373f413e42c5

Comment 2

[Florian Quèze [:florian]]

Some profiles captured with the try build from comment 1:
https://perfht.ml/2h9VHEo
https://perfht.ml/2hba1g8

I think the patch has the intended effect, as the nsHandlerService-json.js stuff is all after we have first painted the preferences page. I haven't managed to reproduce the 4s delay I had seen in bug 1398597 comment 14, but that's not really a surprise given the random aspect of I/O costs.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8908901 [details] [diff] [review]
yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

Review of attachment 8908901 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I like it, thanks :-).

Unsure if this means we should wontfix bug 1400117 in favor of this.

::: browser/components/preferences/in-content/main.js
@@ +446,5 @@
>      // 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?
> +    promiseLoadHandlersList = new Promise((resolve, reject) => {
> +      var _delayedPaneLoad = async function(self) {

While we are touching this code, I would use an arrow function and remove the 'self' parameter. And maybe even inline the function to also remove the '_delayedPaneLoad' variable.

@@ +457,5 @@
> +          reject(ex);
> +        }
> +        resolve();
> +      }
> +      setTimeout(_delayedPaneLoad, 0, gMainPane);

In bug 1400117 I suggested using a pageshow event listener. Do you think that should be done in addition to your patch, or are the sleep calls in your code enough to no longer have any benefit to delaying initialization?

@@ +1521,5 @@
>      for (let type in this._handledTypes) {
> +      // Yield before processing each handler info object to avoid monopolizing
> +      // the main thread, as the objects are retrieved lazily, and retrieval
> +      // can be expensive on Windows.
> +      await sleep(0);

what about:
  await new Promise(resolve => Services.tm.dispatchToMainThread(resolve));
?

Comment 4

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

Attachment: yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

(In reply to Florian Quèze [:florian] [:flo] from comment #3)

> ::: browser/components/preferences/in-content/main.js
> @@ +446,5 @@
> >      // 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?
> > +    promiseLoadHandlersList = new Promise((resolve, reject) => {
> > +      var _delayedPaneLoad = async function(self) {
> 
> While we are touching this code, I would use an arrow function and remove
> the 'self' parameter. And maybe even inline the function to also remove the
> '_delayedPaneLoad' variable.

That makes sense, I've made all these changes.


> @@ +457,5 @@
> > +          reject(ex);
> > +        }
> > +        resolve();
> > +      }
> > +      setTimeout(_delayedPaneLoad, 0, gMainPane);
> 
> In bug 1400117 I suggested using a pageshow event listener. Do you think
> that should be done in addition to your patch, or are the sleep calls in
> your code enough to no longer have any benefit to delaying initialization?

This patch may be sufficient, but I think we should take the change in bug 1400117 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 that 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 "to ensure it doesn't delay painting of the preferences page."  I'll comment in that bug.


> @@ +1521,5 @@
> >      for (let type in this._handledTypes) {
> > +      // Yield before processing each handler info object to avoid monopolizing
> > +      // the main thread, as the objects are retrieved lazily, and retrieval
> > +      // can be expensive on Windows.
> > +      await sleep(0);
> 
> what about:
>   await new Promise(resolve => Services.tm.dispatchToMainThread(resolve));
> ?

Indeed, that's much better; fixed.

-myk

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8909517 [details] [diff] [review]
yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

Review of attachment 8909517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

Comment 6

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

Attachment: yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

This version of the patch resolves conflicts with the fix for bug 1400117.  It also moves the resolve() call into the try block to avoid calling resolve() after calling reject() in the event that the try block throws an exception.

(Calling resolve() after reject() would be ok, since resolve() does nothing in that case; but it's illogical.)

This is the version that I'll push to inbound once the fix for bug 1400117 lands there.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8909980 [details] [diff] [review]
yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

Review of attachment 8909980 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/main.js
@@ +447,5 @@
>      // Load the data and build the list of handlers.
> +    // By doing this after pageshow, we ensure it doesn't delay painting
> +    // of the preferences page.
> +    promiseLoadHandlersList = new Promise((resolve, reject) => {
> +      window.addEventListener("pageshow", async () => {

I wonder if I should have requested a {once: true} option on this addEventListener call just to be safe (not sure if this 'pageshow' event can be fired more than once).

Comment 8

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

Attachment: yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> I wonder if I should have requested a {once: true} option on this
> addEventListener call just to be safe (not sure if this 'pageshow' event can
> be fired more than once).

I don't think it's currently fired more than once, as it looks like about:preferences is reloaded from scratch every time you navigate backward/forward to it in a tab's history (presumably because it registers an unload event, which causes it not to get cached in the bfcache).

However, that could change in the future, so it's safer to request once: true.  Here's an updated patch for this bug that adds that.  How does this look?

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8909998 [details] [diff] [review]
yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo

Review of attachment 8909998 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Myk Melez [:myk] [@mykmelez] from comment #8)

> However, that could change in the future, so it's safer to request once:
> true.  Here's an updated patch for this bug that adds that.  How does this
> look?

Looks good, and should land soon if we want this in 57 :-). Thanks!

Comment 10

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

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> Looks good, and should land soon if we want this in 57 :-). Thanks!

Yep, I've just been waiting for autoland to merge to central to merge to inbound, so I can apply this on top of the fix for bug 1400117 and push it to inbound.  Those merges just happened, so I've applied this and am doing one last build/test locally before pushing to inbound.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4940894b7ef5
yield thread between each call to gExternalProtocolService.getProtocolHandlerInfo; r=florian

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4940894b7ef5