842627 - [window manager] don't take up resources while the app is loading (cold launch)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Etienne Segonzac (:etienne)]

We do a bunch of stuff in the WindowManager during the launch of an app. But when the poor little app is busy loading we shouldn't take up any resources.

The (rather simple) change is to wait for the window to be loaded & painted before taking a screenshot etc...

Comment 1

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Note: moving the profiling event to the capturing phase does not give us any fake gain by itself.

And the gain here is 80-100ms for all apps.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 715560 [details] [diff] [review]
Patch proposal

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

Sounds good let's land that and try it as soon as possible :)

Comment 3

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/8424d6e3860759f75c2db9161bacfe1e98d7ea0b

Comment 4

[Lucas Adamski [:ladamski]]

I'd love a 80-100ms win for all apps.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

We will mark blocking here, please land today and if there are any regressions of breakage we will have to push this back to v1-train only.

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train@cef39f8d6b6fda1255891cbe507c9df38be1fdbc
v1.0.1@cac6fb37e84c018491064f9da4765533dbfb769a

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

This one introduces bug 844990.

If you guys still love this performance change,
I propose to put only screenshot work later instead of the whole windowOpened() function.

I don't see the reason we shall delay:
1. Set variable 'displayedApp'
2. Request orientation
3. Dispatch the appopen event

..after the app content is loaded.
The above is just something critial and only relevant to "window" itself in Window Manager.
I guess they are costing less than getScreenshot() function.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This has been backouted by bug 844990 in https://github.com/mozilla-b2g/gaia/pull/8597. Let's reopen.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alive Kuo [:alive] (PTO during 3/27~4/7) from comment #7)
> This one introduces bug 844990.
> 
> If you guys still love this performance change,
> I propose to put only screenshot work later instead of the whole
> windowOpened() function.
> 
> I don't see the reason we shall delay:
> 1. Set variable 'displayedApp'
> 2. Request orientation

I agree for those 2 points.

> 3. Dispatch the appopen event

I don't see why appopen is critical for a newly opened app. It is for an application that is already opened though.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v2

This is a safer version of the patch. Alive, Tim since you guys are in vacation I will ask a r? to Etienne.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> (In reply to Alive Kuo [:alive] (PTO during 3/27~4/7) from comment #7)
> > This one introduces bug 844990.
> 
> I don't see why appopen is critical for a newly opened app. It is for an
> application that is already opened though.

Hey,

IMO appopen is just an event to say: the app transition is over, but the content may be still loading.
If you want another event to tell the content loads ended, isn't a new event more reasonable on semantic?

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive] (PTO during 3/27~4/7) from comment #11)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> > (In reply to Alive Kuo [:alive] (PTO during 3/27~4/7) from comment #7)
> > > This one introduces bug 844990.
> > 
> > I don't see why appopen is critical for a newly opened app. It is for an
> > application that is already opened though.
> 
> Hey,
> 
> IMO appopen is just an event to say: the app transition is over, but the
> content may be still loading.
> If you want another event to tell the content loads ended, isn't a new event
> more reasonable on semantic?

I think the rationale isn't that we want to explicitly wait for the loadend, but rather that, since event dispatching is synchronous, we want all this potential work triggered by "appopen" to happen out of the app loading phase.

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 731161 [details] [diff] [review]
Patch v2

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

r=me with nits.

This is indeed safer, and definitely not bringing back bug 844990.

::: apps/system/js/window_manager.js
@@ +396,5 @@
> +        setTimeout(openCallback);
> +        openCallback = null;
> +        setOpenFrame(null);
> +      };
> + 

I don't want Travis to see this.

@@ +401,5 @@
> +      // If this is a cold launch let's wait for the app to load first
> +      var iframe = openFrame.firstChild;
> +      if ('unpainted' in iframe.dataset) {
> +
> +        if ('wrapper' in frame.dataset)

{

@@ +402,5 @@
> +      var iframe = openFrame.firstChild;
> +      if ('unpainted' in iframe.dataset) {
> +
> +        if ('wrapper' in frame.dataset)
> +          wrapperFooter.classList.add('visible');

}

Comment 14

[Etienne Segonzac (:etienne)]

If Alive is okay with the explanation in Comment 12 this is safe to land.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #14)
> If Alive is okay with the explanation in Comment 12 this is safe to land.

:)

I don't look into the patch yet.

I know the original idea of the proposal by comment 0 is thoughtful and interesting.
Just want to make sure we are careful to do this because regression like 844990 is not obvious when we do such change. What I said in comment 7 and comment 11 is, for example, if something is needed to happen before load-end but after appopen event, that may be a candidate of regression.

Anyway it's surely ok to land an r+ patch because we already know the existence of this change. (Means if any regression is there we'll easily figure out what happens.)

Thank you all.

Comment 16

[Joe Cheng [:jcheng] (please needinfo)]

is this ready to land? thanks

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/996cbf21d41e02cf4ba0e7445859838c5db7bdf7

Comment 18

[Julien Wajsberg [:julienw]]

should this be uplifted to v1-train and v1.0.1 ? According to datazilla all apps (except browser) recently got a 100ms improvement, and this patch is my culprit.

We can wait a few days to see if nothing breaks on master.

Comment 19

[Julien Wajsberg [:julienw]]

asking tef? again as we're more than 1 month after the previous setting.

The patch looks like safer than the previous one.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I put unaffected mostly because this flag has been set after the patch has landed on v1-train but has not been removed when it has been backed out. I still believe this needs to land to v1-train.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking so this can be prioritized for v1-train landing while tef? is decided.

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 731161 [details] [diff] [review]
Patch v2

Asking for approval then.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: 100ms less when starting an app
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low-medium; it's central to gaia but this landed 5 days ago already and we probably would have caught the biggest regressions if any. Further QA will be done on v1-train.
String or UUID changes made by this patch: none

Comment 23

[Alex Keybl [:akeybl]]

(In reply to Julien Wajsberg [:julienw] from comment #22)
> Risk to taking this patch (and alternatives if risky): low-medium; it's
> central to gaia but this landed 5 days ago already and we probably would
> have caught the biggest regressions if any. Further QA will be done on
> v1-train.

v1.1 doesn't have any new perf requirements over v1.0.1 (if this doesn't end up being tef+), but we may choose to approve still if you think regressions will be easily caught by QA through app switch/launch. Does that line up with your thinking?

Comment 24

[Julien Wajsberg [:julienw]]

The testcases should be :

- launch app
- switch app using the switcher
- switch app using an activity
- launch app, press home quick
- launch app, press home quick, launch same app
- launch app, press home quick, launch another app

I think that's it.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 731161 [details] [diff] [review]
Patch v2

Approving and will set verifyme in order to ensure QA testing around this.  Is there going to be any automation around this to track the perf wins here and prevent regressions to this improvement?

Comment 26

[Julien Wajsberg [:julienw]]

Lucas, the automation for perf testing is already there. We clearly see the perf improvement on datazilla, near the 5th april for the master branch.

Comment 27

[Julien Wajsberg [:julienw]]

prepared the uplift to v1-train: https://github.com/julienw/gaia/tree/842627-uplift-v1-train

But I want to test it on a device before pushing because I'm not comfortable with this conflict resolution.

Comment 28

[Julien Wajsberg [:julienw]]

v1-train: 5e94419dd97344922e202cdebd03dc4570949c1c
v1.0.1: c79e761bae4d92f329154c64159f4f5c8eb49c9e

Comment 29

[Julien Wajsberg [:julienw]]

sorry, I reset the flag because I wasn't paying attention, please put it back to tef+.

Comment 30

[sarsenyev]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?