1117444 - [SMS][dolphin][FFOS7715 v2.1][performance] Close sms app spend a longer time

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect)

Description

[Wei Gao (Spreadtrum)]

*** Build Information
Gaia-Rev        73be51f998031f06db0cd660c0e388fa621c9f4c
Gecko-Rev       30de9395e2c3e4c3d640bc6c70ddbc1a8c8cf88f
Device-Name     dolphin

*** Steps to Reproduce
1.Fill the device with 250 contacts, 50 call logs, 100 sms, 300 images, 150 music, 50 video.
2.Open Sms.
3.Close Sms app and hide it to background. 

*** Expected Results
The sms app can be hided promptly.

*** Actual Results
The performance of Sms close in FFOS2.1 is worse than android.

The average time
7715-FFOS2.1 : 0.933
7715-android : 0.683

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

This is purely a Gaia::System issue.

Hey Alive, I NI you just so that you know about this bug :)

Comment 2

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

Should be perf component.

Comment 3

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Per discussion with partner, the current performance is good enough. Hence close this one as won't fix

Comment 5

[GitHub Autolander Bot]

Attachment: [PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

Hey Chris, sorry for all the reviews, but this is at the intersection of system/homescreen/gfx so you're definitely the best person to ask :)

When going back to the homescreen from an app, we wait for the homescreen to paint before starting the transition (because we want the grid of icons zomming in to be part of the transition), see [1] and [2].

This routinely takes 400-500ms :/
I'm seeing a solid 100-150ms gain with this patch (left the console.time in), does it make sense? Can we do even better?

Cheers!

PS: also realized we probably do something really wrong when going back to the homescreen as soon as the app opened (ie. the icons are *never* there for the transition), the patch doesn't address this case (should probably file a separate bug for it).

[1] https://github.com/etiennesegonzac/gaia/blob/master/apps/system/js/app_window_manager.js#L247
[2] https://github.com/etiennesegonzac/gaia/blob/master/apps/system/js/app_window.js#L1825

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

Yes, this makes sense - although it flags some possible gfx issue, removing the overflow on something with a displayport will always reduce the amount of work there is, even under ideal conditions. As long as we make sure we don't do this during a period where the user may interact with the frame, this should be fine.

One thing I would worry about is the redraw lag now happening after the app presents itself, rather than before, and introducing a period where shortly after the app shows, you can't interact with it. (rather, a longer period) I think though that if this is only in the region of <~350ms, we can get away with it.

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #7)
> Comment on attachment 8559899 [details] [review]
> [PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master
> 
> Yes, this makes sense - although it flags some possible gfx issue, removing
> the overflow on something with a displayport will always reduce the amount
> of work there is, even under ideal conditions. As long as we make sure we
> don't do this during a period where the user may interact with the frame,
> this should be fine.
> 
> One thing I would worry about is the redraw lag now happening after the app
> presents itself, rather than before, and introducing a period where shortly
> after the app shows, you can't interact with it. (rather, a longer period) I
> think though that if this is only in the region of <~350ms, we can get away
> with it.

Re the icons never appearing, I think we have async image decoding, right? I don't know anything about that, but maybe there's some signal we can listen for (or perhaps a signal we need to introduce on the browser element) for when the images are ready after the frame is presented? I think someone with this specific gfx/layout knowledge needs to weigh in on that.

Comment 9

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Reopening as Etienne is working on this.

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

System + Homescreen, but pretty nice result :)

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

I have some questions, but I think this can go in - you'll definitely want Alive's review though, he's much more knowledgeable about these parts of the code.

Comment 12

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

Will review this today; wondering if there is a better way to achieve that.

Comment 13

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

Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

r+ for appWindow change; hopefully there is an elegant way to do it but can't figure out how for now.

Comment 14

[Etienne Segonzac (:etienne)]

Just updated the PR but looks like this is causing a bad launch time regression (~200ms), tested with the sms app.

Probably because we setVisible(false) (and gc) the homescreen while the app is launcher as opposed to before it starts...

I should maybe remove the setVisible(false) delaying from the patch.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

Updated the patch. The homescreen setVisible delay and the setTimeout removals where causing the launch time regression.

This makes for a sweet and simple patch (especially with -w [1]) but I wouldn't mind a last sanity check.

[1] https://github.com/mozilla-b2g/gaia/pull/27976/files?w=1

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

LGTM.

Comment 17

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 18

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/d56735ed3fdb925357431ef3808e66b1228f9c25

Comment 19

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Is it worth asking an approval ?

Comment 20

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> Is it worth asking an approval ?

Don't think the gain is big enough to go through the hassle.