Closed Bug 1212321 Opened 4 years ago Closed 4 years ago

Stop painting on app launch

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(firefox45 fixed, firefox46 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: vnicolas, Assigned: vnicolas, NeedInfo)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
Attachment #8670770 - Flags: review?(fabrice)
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?
Attachment #8670770 - Flags: review?(fabrice) → feedback+
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?
(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.
(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 ?
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.
Attachment #8670770 - Attachment is obsolete: true
Attachment #8671245 - Flags: review?(fabrice)
When the app goes to background is still too early. Let's use a timer instead.
Attachment #8671245 - Attachment is obsolete: true
Attachment #8671245 - Flags: review?(fabrice)
Attachment #8671346 - Flags: review?(fabrice)
Vivien, can you run raptor locally to check how much we gain from this patch?
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 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
Attachment #8671346 - Flags: review?(fabrice) → review+
Assignee: nobody → vnicolas
Keywords: checkin-needed
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!
Flags: needinfo?(vnicolas)
Keywords: checkin-needed
Attached patch Patch to check-in (obsolete) — Splinter Review
Flags: needinfo?(vnicolas)
Keywords: checkin-needed
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 ?
Flags: needinfo?(vnicolas)
Attached patch Patch to check-in. (obsolete) — Splinter Review
Seems like I'm unlucky with this patch.
Attachment #8683049 - Attachment is obsolete: true
Flags: needinfo?(vnicolas)
Keywords: checkin-needed
Attachment #8684135 - Attachment description: stop.painting.on.applaunch.hack.patch → Patch to check-in.
https://hg.mozilla.org/mozilla-central/rev/50441cd0d6a1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
That's now broken because pausePainting() and resumePainting() were removed in bug 1059014 :(
Flags: needinfo?(21)
(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.
Depends on: 1226284
See Also: → 1227424
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.
Talked IRL with Vivien.

Can we please backout this patch ?

Thanks !
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
(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
This patch is really killing me.
Flags: needinfo?(21)
Keywords: checkin-needed
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.
Attachment #8671346 - Attachment is obsolete: true
Attachment #8684135 - Attachment is obsolete: true
Attachment #8691927 - Flags: review?(dbaron)
Attached patch stop.painting.on.applaunch.patch (obsolete) — Splinter Review
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.
Attachment #8691929 - Flags: review?(fabrice)
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?
Attachment #8691929 - Flags: review?(fabrice)
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.
Attachment #8691927 - Flags: review?(dbaron) → review+
Duplicate of this bug: 1226284
Seems like freezing up the parent process was a bad idea :)
Attachment #8691929 - Attachment is obsolete: true
Attachment #8692924 - Flags: review?(fabrice)
Attachment #8692924 - Flags: review?(fabrice) → review+
Let's cross fingers for the checkin this time...
Keywords: checkin-needed
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
Flags: needinfo?(vnicolas)
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/a95b36044ed8
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.