958318 - Settings Takes 200ms Finding Homescreen Counts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P1)

Description

[Mason Chang [Inactive] [:mchang]]

Attachment: 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.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: 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

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15175

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15178

Comment 4

[Fabien Cazenave [: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.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Profile of Settings App w/ Patch

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15219

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: please nominate for approval.

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Profile of Settings App w/ Patch

Down to 51 ms im try homescreen count.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15317

Comment 11

[Kevin Grandon :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.

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Please check my github comments, thanks!

Comment 13

[Mason Chang [Inactive] [:mchang]]

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

Updated with your feedback.

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

r=me. Thanks!

Comment 15

[Mason Chang [Inactive] [:mchang]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15419

Carrying r+ and f+ from arthurcc and kgrandon.

Comment 16

[Mason Chang [Inactive] [:mchang]]

Landed:

master: 13e6907011212fffe6284862638aa84d4b407123
v1.3: 6d0a735f4e69c2151980afa38c05050ef74beb96


Thanks arthur!

Comment 17

[Mason Chang [Inactive] [:mchang]]

Requesting v1.3+ status as this solved bug 951221.