If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Settings Takes 200ms Finding Homescreen Counts

RESOLVED FIXED in Firefox OS v1.3

Status

Firefox OS
Gaia::Settings
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

({perf})

unspecified
1.3 C2/1.4 S2(17jan)
x86
Mac OS X

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [c=handeye p=3 s= u=1.3])

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8358060 [details]
Profile of Settings App

From the attached profile, we see we spend 218 ms loading an optional homescreen setting by iterating through all the apps to see if they homescreens. This causes large jank after a cold launch.
(Assignee)

Comment 1

4 years ago
Created attachment 8358062 [details]
Profile of Settings App w/ Patch

We remove the homescreen search from the startup path, going from 218 ms down to 54 ms, saving 164 ms
(Assignee)

Comment 2

4 years ago
Created attachment 8358074 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15175
(Assignee)

Comment 3

4 years ago
Created attachment 8358165 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15178
Attachment #8358074 - Attachment is obsolete: true
Attachment #8358165 - Flags: review?(kaze)
Comment on attachment 8358165 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15178

Adding an arbitrary 5s timeout doesn’t sound like the best possible fix. Using non-zero timeouts is not healthy.

If that’s really the only patch you can propose for this bug, we’d need:
 • a global all-caps constant at the beginning of the file instead of a local “delay” variable;
 • a comment explaining why we rely on such a hack;
 • another reviewer (e.g. :arthurcc), because I’d probably still not r+ it. :-p

As I wrote on github, I think you probably want to use a callback or an event listener instead.
Attachment #8358165 - Flags: review?(kaze) → review-
(Assignee)

Comment 5

4 years ago
Created attachment 8358663 [details]
Profile of Settings App w/ Patch
Attachment #8358062 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 8358695 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15219
Attachment #8358165 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8358695 - Flags: review?(kaze)
Attachment #8358695 - Flags: review?(arthur.chen)
(Assignee)

Updated

4 years ago
blocking-b2g: --- → 1.3?
Triage: please nominate for approval.
blocking-b2g: 1.3? → -
Comment on attachment 8358695 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15219

Thanks for finding the performance hotspot and the patch. A few comments:

1. Can we add an idle observer in try_show_homescreen_section.js so that we don't need the custom event?
2. We don't need to store the installed homescreen count in the async storage as we always check it every time we launch settings app.
3. To save the refresh time when app installed/uninstalled, we can only check if the installed/uninstalled app is a homescreen app or not instead of get all apps again.
Attachment #8358695 - Flags: review?(arthur.chen)
(Assignee)

Comment 9

4 years ago
Created attachment 8359925 [details]
Profile of Settings App w/ Patch

Down to 51 ms im try homescreen count.
Attachment #8358663 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8359927 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15317
Attachment #8358695 - Attachment is obsolete: true
Attachment #8358695 - Flags: review?(kaze)
Attachment #8359927 - Flags: review?(kaze)
Attachment #8359927 - Flags: review?(arthur.chen)
(Assignee)

Updated

4 years ago
Attachment #8359927 - Flags: feedback?(kgrandon)
Comment on attachment 8359927 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15317

I hate that we have to do this, but you can't deny the benefit. I wonder if there is some nicer long term approach, maybe caching the HTML somehow.
Attachment #8359927 - Flags: feedback?(kgrandon) → feedback+
Comment on attachment 8359927 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15317

Please check my github comments, thanks!
Attachment #8359927 - Flags: review?(arthur.chen)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8359927 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15317

Updated with your feedback.
Attachment #8359927 - Flags: review?(kaze) → review?(arthur.chen)
Comment on attachment 8359927 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15317

r=me. Thanks!
Attachment #8359927 - Flags: review?(arthur.chen) → review+
(Assignee)

Comment 15

4 years ago
Created attachment 8361200 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15419

Carrying r+ and f+ from arthurcc and kgrandon.
Attachment #8359927 - Attachment is obsolete: true
Attachment #8361200 - Flags: review+
(Assignee)

Comment 16

4 years ago
Landed:

master: 13e6907011212fffe6284862638aa84d4b407123
v1.3: 6d0a735f4e69c2151980afa38c05050ef74beb96


Thanks arthur!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
status-b2g-v1.3: --- → fixed
(Assignee)

Comment 17

4 years ago
Requesting v1.3+ status as this solved bug 951221.
blocking-b2g: - → 1.3?

Updated

4 years ago
blocking-b2g: 1.3? → 1.3+
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.