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

RESOLVED FIXED

Status

Firefox OS
Performance
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Wei Gao (Spreadtrum), Unassigned)

Tracking

(Blocks: 1 bug, {perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sprd388709][dp3])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
*** 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
(Reporter)

Updated

3 years ago
Whiteboard: [sprd388709]
Keywords: perf
This is purely a Gaia::System issue.

Hey Alive, I NI you just so that you know about this bug :)
Component: Gaia::SMS → Gaia::System::Window Mgmt
Flags: needinfo?(alive)
Should be perf component.
Component: Gaia::System::Window Mgmt → Performance
Flags: needinfo?(alive)

Updated

3 years ago
Whiteboard: [sprd388709] → [sprd388709][dp3]

Updated

3 years ago
Blocks: 1123554
Per discussion with partner, the current performance is good enough. Hence close this one as won't fix
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Duplicate of this bug: 1128583
Created attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master
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
Attachment #8559899 - Flags: feedback?(chrislord.net)

Comment 7

3 years ago
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.
Attachment #8559899 - Flags: feedback?(chrislord.net) → feedback+

Comment 8

3 years ago
(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.
Reopening as Etienne is working on this.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

System + Homescreen, but pretty nice result :)
Attachment #8559899 - Flags: review?(chrislord.net)
Attachment #8559899 - Flags: review?(alive)
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.
Attachment #8559899 - Flags: review?(chrislord.net) → review+
Will review this today; wondering if there is a better way to achieve that.
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.
Attachment #8559899 - Flags: review?(alive) → review+
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 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
Attachment #8559899 - Flags: review+ → review?(chrislord.net)
Comment on attachment 8559899 [details] [review]
[PullReq] etiennesegonzac:bug-1117444 to mozilla-b2g:master

LGTM.
Attachment #8559899 - Flags: review?(chrislord.net) → review+
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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.
https://github.com/mozilla-b2g/gaia/commit/d56735ed3fdb925357431ef3808e66b1228f9c25
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Is it worth asking an approval ?
(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.
You need to log in before you can comment on or make changes to this bug.