1212321 - Stop painting on app launch

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[vnicolas]

Attachment: stop.painting.on.applaunch.hack.patch

I noticed in the profiler that repainting the app that launch the app slow down app startup.
There are 2 reasons why we are repainting in the homescreen app from what I can see:
 1. We are toggling a class that change some opacity on the icon
 2. The platform is doing some work when the mouseup event is received

I already fixed #1 in the past. It has came back...
For #2 there is no content side solution.

Also third party homescreen may do things that may slow down app startup. This patch prevents repainting from the platform side when an application is launched.

Comment 1

[[:fabrice] Fabrice Desré]

Comment on attachment 8670770 [details] [diff] [review]
stop.painting.on.applaunch.hack.patch

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

::: dom/apps/Webapps.js
@@ +579,5 @@
>                                                oid: this._id,
>                                                topId: this._topId,
>                                                timestamp: Date.now(),
>                                                requestID: this.getRequestId(request) });
> +    Services.obs.notifyObservers(null, 'will-launch-app', null);

nit: double quotes in this file.

@@ +581,5 @@
>                                                timestamp: Date.now(),
>                                                requestID: this.getRequestId(request) });
> +    Services.obs.notifyObservers(null, 'will-launch-app', null);
> +    this.addMessageListeners(["Webapps:Launch:Return:OK",
> +                              "Webapps:Launch:Return:KO"]);

why did you move that?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8670770 [details] [diff] [review]
stop.painting.on.applaunch.hack.patch

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +322,5 @@
> +            return;
> +          }
> +
> +          removeEventListener(e.type, onVisible);
> +          docShell.contentViewer.resumePainting();

This means we will stop the refresh driver until the homescreen is foreground again right? I guess rAF will also be stopped. Will this have negative effect?

Comment 3

[vnicolas]

(In reply to [:fabrice] Fabrice Desré from comment #1)
> Comment on attachment 8670770 [details] [diff] [review]
> stop.painting.on.applaunch.hack.patch
> 
> Review of attachment 8670770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/Webapps.js
> @@ +579,5 @@
> >                                                oid: this._id,
> >                                                topId: this._topId,
> >                                                timestamp: Date.now(),
> >                                                requestID: this.getRequestId(request) });
> > +    Services.obs.notifyObservers(null, 'will-launch-app', null);
> 
> nit: double quotes in this file.

Grr. I hate our mix of double versus single quote. I keep mixing them up. Will fix that.

> 
> @@ +581,5 @@
> >                                                timestamp: Date.now(),
> >                                                requestID: this.getRequestId(request) });
> > +    Services.obs.notifyObservers(null, 'will-launch-app', null);
> > +    this.addMessageListeners(["Webapps:Launch:Return:OK",
> > +                              "Webapps:Launch:Return:KO"]);
> 
> why did you move that?

This is a bit a micro-optimization. I can see this call in the profiler sometimes. Since it happens before the code that ask to dispatch the message to the other process, I moved it a few lines to make sure it does not slow down app launch. It is really a story of 1 or 2 ms.

Comment 4

[vnicolas]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #2)
> Comment on attachment 8670770 [details] [diff] [review]
> stop.painting.on.applaunch.hack.patch
> 
> Review of attachment 8670770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +322,5 @@
> > +            return;
> > +          }
> > +
> > +          removeEventListener(e.type, onVisible);
> > +          docShell.contentViewer.resumePainting();
> 
> This means we will stop the refresh driver until the homescreen is
> foreground again right? I guess rAF will also be stopped. Will this have
> negative effect?

Afaict it will stop the refresh driver as you mentioned. 

For raF I just tried and you are correct again. For non-homescreen background apps raF keeps firing at a very slow rate. My main goal here is to ensure app-launch is not slow down.
I can change the code so resumePainting will be called not when the homescreen goes foreground again, but when the homescreen goes background. What do you think ?

Comment 5

[vnicolas]

Attachment: stop.painting.on.applaunch.hack.patch

Change the code to only call contentViewer.pausePainting() if the app is the docShell is active and call contentViewer.resumePainting() as soon it becomes inactive.

Comment 6

[vnicolas]

Attachment: stop.painting.on.applaunch.hack.patch

When the app goes to background is still too early. Let's use a timer instead.

Comment 7

[[:fabrice] Fabrice Desré]

Vivien, can you run raptor locally to check how much we gain from this patch?

Comment 8

[vnicolas]

A little bit less than a 30ms win for all metrics on Aries. Likely more on Flame.

Here are the results for:
 - raptor test coldlaunch --app clock --runs 10

Without the patch:

| Metric                           | Mean    | Median  | Min     | Max     | StdDev | p95     |
| -------------------------------- | ------- | ------- | ------- | ------- | ------ | ------- |
| coldlaunch.navigationLoaded      | 419.600 | 403.500 | 388.000 | 507.000 | 33.664 | 507.000 |
| coldlaunch.navigationInteractive | 493.600 | 487.500 | 459.000 | 574.000 | 31.664 | 574.000 |
| coldlaunch.visuallyLoaded        | 636.600 | 644.000 | 579.000 | 724.000 | 39.134 | 724.000 |
| coldlaunch.contentInteractive    | 636.600 | 644.000 | 579.000 | 724.000 | 39.134 | 724.000 |
| coldlaunch.fullyLoaded           | 636.600 | 644.000 | 579.000 | 724.000 | 39.134 | 724.000 |
| coldlaunch.uss                   | 12.230  | 12.300  | 11.400  | 12.500  | 0.283  | 12.500  |
| coldlaunch.pss                   | 17.670  | 17.700  | 16.900  | 17.900  | 0.265  | 17.900  |
| coldlaunch.rss                   | 31.620  | 31.700  | 30.900  | 31.900  | 0.252  | 31.900  |


With the patch:

| Metric                           | Mean    | Median  | Min     | Max     | StdDev | p95     |
| -------------------------------- | ------- | ------- | ------- | ------- | ------ | ------- |
| coldlaunch.navigationLoaded      | 391.900 | 395.500 | 356.000 | 432.000 | 21.934 | 432.000 |
| coldlaunch.navigationInteractive | 465.700 | 469.000 | 434.000 | 511.000 | 22.249 | 511.000 |
| coldlaunch.visuallyLoaded        | 608.600 | 603.500 | 558.000 | 676.000 | 33.341 | 676.000 |
| coldlaunch.contentInteractive    | 608.800 | 604.000 | 558.000 | 676.000 | 33.223 | 676.000 |
| coldlaunch.fullyLoaded           | 608.900 | 604.000 | 559.000 | 676.000 | 33.071 | 676.000 |
| coldlaunch.uss                   | 12.230  | 12.300  | 11.700  | 12.300  | 0.179  | 12.300  |
| coldlaunch.pss                   | 17.650  | 17.700  | 17.100  | 17.800  | 0.186  | 17.800  |
| coldlaunch.rss                   | 31.630  | 31.700  | 31.100  | 31.700  | 0.179  | 31.700  |

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8671346 [details] [diff] [review]
stop.painting.on.applaunch.hack.patch

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +20,5 @@
>  let kLongestReturnedString = 128;
>  
> +const Timer = Components.Constructor("@mozilla.org/timer;1",
> +                                     "nsITimer",
> +                                     "initWithCallback");

nit: add blank line

Comment 10

[Carsten Book [:Tomcat]]

Hi, this patch didn't apply:

renamed 1212321 -> stop.painting.on.applaunch.hack.patch
applying stop.painting.on.applaunch.hack.patch
patching file dom/browser-element/BrowserElementChildPreload.js
Hunk #1 FAILED at 13
Hunk #2 FAILED at 51
Hunk #3 FAILED at 280
3 out of 5 hunks FAILED -- saving rejects to file dom/browser-element/BrowserElementChildPreload.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh stop.painting.on.applaunch.hack.patch

could you take a look, thanks!

Comment 11

[vnicolas]

Attachment: Patch to check-in

Comment 12

[Carsten Book [:Tomcat]]

Hm still failed to apply:

renamed 1212321 -> stop.painting.on.applaunch.hack.patch
applying stop.painting.on.applaunch.hack.patch
patching file dom/browser-element/BrowserElementChildPreload.js
Hunk #1 FAILED at 16
1 out of 5 hunks FAILED -- saving rejects to file dom/browser-element/BrowserElementChildPreload.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh stop.painting.on.applaunch.hack.patch

maybe need a rebase to mozilla-inbound ?

Comment 13

[vnicolas]

Attachment: Patch to check-in.

Seems like I'm unlucky with this patch.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50441cd0d6a1

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50441cd0d6a1

Comment 16

[[:fabrice] Fabrice Desré]

That's now broken because pausePainting() and resumePainting() were removed in bug 1059014 :(

Comment 17

[vnicolas]

(In reply to [:fabrice] Fabrice Desré from comment #16)
> That's now broken because pausePainting() and resumePainting() were removed
> in bug 1059014 :(

Sigh. I will fix that.

Comment 18

[Julien Wajsberg [:julienw]]

Hey Vivien,

I think this bug is the real cause for bug 1227424. Moreover it causes bug 1226284 and has no effect because of bug 1059014.

To make things easier to follow, how about:
* backout this patch first, because anyway it's useless currently.
* reland bug 1059014 part 3 as part of this bug

This will make things clearer, especially see if this is the real cause for bug 1227424 and help us debug this issue.

Comment 19

[Julien Wajsberg [:julienw]]

Talked IRL with Vivien.

Can we please backout this patch ?

Thanks !

Comment 20

[Carsten Book [:Tomcat]]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> Talked IRL with Vivien.
> 
> Can we please backout this patch ?
> 
> Thanks !

backed out on request in https://hg.mozilla.org/mozilla-central/rev/1c35ad64bf2f

Comment 21

[vnicolas]

This patch is really killing me.

Comment 22

[vnicolas]

Attachment: add.back.pausePainting.resumePainting.patch

This is basically a backout of https://bug1059014.bmoattachments.org/attachment.cgi?id=8683477
The only changes are the uuid since they have been updated in the meantime. So I have regenerated some uuids.

Comment 23

[vnicolas]

Attachment: stop.painting.on.applaunch.patch

The integration test failure happens when an application calls navigator.mozApps.getSelf().then((app) => app.launch()) while the app is already visible...
Once it happens, the refresh driver is paused and so we ends up missing an animationend event which is never fired. It is a bit sad that animationend is never fired and I think this is similar to bug 683696 as those events are related to when the refresh driver ticks, but this patch freeze it (i may have misunderstood how it works here).

So the new change in this patch is to avoid calling will-launch-app is the targetted app is the same as the emitter of the app.launch() call.

Comment 24

[vnicolas]

Comment on attachment 8691929 [details] [diff] [review]
stop.painting.on.applaunch.patch

I have seen a bug with the homescreen - will fix it before asking a new r?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8691927 [details] [diff] [review]
add.back.pausePainting.resumePainting.patch

I already said this was fine to back out in bug 1059014 comment 16, but sure.

Comment 27

[vnicolas]

Attachment: stop.painting.on.applaunch.patch

Seems like freezing up the parent process was a bad idea :)

Comment 28

[vnicolas]

Let's cross fingers for the checkin this time...

Comment 29

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

applying add.back.pausePainting.resumePainting.patch
patching file layout/base/nsIPresShell.h
Hunk #1 FAILED at 134
1 out of 3 hunks FAILED -- saving rejects to file layout/base/nsIPresShell.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh add.back.pausePainting.resumePainting.patch

Comment 30

[Nicholas Nethercote [inactive]]

The failure is probably just due to UUIDs changing. Vivien, it's probably worth you checking it in yourself rather than using checkin-needed to minimize the chance of the UUIDs changing and causing further conflicts.

Comment 31

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/a95b36044ed8

Comment 32

[Carsten Book [:Tomcat]]

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