842215 - Poor scrolling FPS in settings for 1-2 seconds after cold start

Status: VERIFIED FIXED

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

Description

[Michael Vines [:m1] [:evilmachines]]

When launching Settings, the UI presents itself before it's really for 'full speed' scrolling.  

STR:
1) Cold start settings
2) As soon as app appears, swipe down like an eager user
3) Notice poor FPS for 1-2 seconds before it becomes butter

#3 used to be more like 4-5 seconds but even the current 1-2 seconds is noticeable and annoying. 

If we have to choose, it may be better to delay presenting the settings app until it's ready for full-speed scrolling.

Comment 1

[Evelyn Hung]

After profiling and a few manually tests, we have some hints:
1. the animation on Geolocation toggle will block scrolling event.
2. it spent a lot of time on reflow and repaint, because we update fields on the main menu after getting the results from mozSettings or device interface.
  - a few notable scripts that hurts scrolling speed: battery.js, app_storage.js, media_storage.js, and icc_menu.js.

I'll look into these scripts more, and try another way to update UI fields tomorrow.

Comment 2

[Evelyn Hung]

I'll remove the initial animation on airplane mode and Geolocation toggles first.

Comment 3

[Evelyn Hung]

no, it seems the animation is not a problem, I tried to set the toggle transition time to 3 seconds, and found I still can scroll down the page while the toggle is switching on.

Comment 4

[Evelyn Hung]

correct myself: the animation on the toggles won't block scrolling but makes it become sluggish, especially at the moment the animation is just applied. (I don't know why.)
I still prefer to remove animation when we are initializing these toggle, because I feel it's weird when a user opens Settings app just for checking his geolocation state, but seeing the toggle slides to the right without any touch.

Since it changes the visual effect, I'd like to hear from UX team. Could we skip the first time (initial setting) animation of all toggles?

Comment 5

[Michael Vines [:m1] [:evilmachines]]

Hey Evelyn, can you please point me to where the animation can be disabled for initial setting?  I'd like to see if that helps minimize this startup lag here

Comment 6

[Evelyn Hung]

another finding: We did a setting cache after bug 840322, but unexpectedly query database twice. One is in `preInit` which is executed before DOM loaded, another is in `init` which is executed after DOM loaded. The second one is triggered in a very short period, so it passes the `if (!this._settingsCache)` check in `getSettings` before the first DOMRequest goes into onsuccess.

Comment 7

[Evelyn Hung]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> Hey Evelyn, can you please point me to where the animation can be disabled
> for initial setting?  I'd like to see if that helps minimize this startup
> lag here
The simplest way is: turn off geolocation and airplane mode (airplane mode should be off by default) in Settings main menu.
There is no a trivial way to disable the animation now, but if you really want to make the CSS take no effect, add rules like below in shared/style/switches.css

label input[data-type="switch"].initial:checked + span,
label input[data-type="switch"].initial:checked + span:before,
label input[data-type="switch"].initial:checked + span:after {
  transition: none;
}

and add a class on geolocation toggle:
<input type="checkbox" data-type="switch" name="geolocation.enabled" class="initial" />

I guess it won't improve startup lag too much? The animation should happens after getting setting value from database (at that moment the app is ready?) and the animation takes 0.2 second now.

Anyway, nice to hear you are tuning the performance issue of Settings app! Thanks.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Screenshotting is showing up on my profiles.  Another reason to turn them off! :)

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With that off, the remaining junk in the profiles is
 - surprising overhead in mouse_event_shim.  Simple tweak made, need to test a bit more though.
 - surprising amount of buffer realloc. Will see what's up there.
 - stepping on several reflows, but they're not too terribly bad

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We're not doing anything /particularly/ bad on startup.  The scene decompositionn we choose initially, we keep during startup.  Buffer sizes and transparency stay the same, so alloc should be coming just from front then back buffer alloc.

There's one issue though --- we end up hitting the colorlayer->thebeslayer deoptimization case, where we keep a thebeslayer instead of optimizing to colorlayer.  This causes us to hold on to an extra layer that needs buffers.

Also, on the first scroll, the partially transparent layer on top of the deoptimized thebeslayer happens to move an opaque header out of view.  This changes the bounds of the are not covered by the top layer, so the deoptimized thebeslayer has to realloc buffers for itself.

It seems like it might be a good idea to stop deoptimizing uniform-color thebeslayers when they force buffer alloc traffic.  But that's too risky to do here.

Vlad, it'd be really awesome to have the powers to upload a layerscope timeline.  Would make the description here easier to follow ;).

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Evelyn Hung [:evelyn] from comment #6)
> another finding: We did a setting cache after bug 840322, but unexpectedly
> query database twice. One is in `preInit` which is executed before DOM
> loaded, another is in `init` which is executed after DOM loaded. The second
> one is triggered in a very short period, so it passes the `if
> (!this._settingsCache)` check in `getSettings` before the first DOMRequest
> goes into onsuccess.

Hah, nice find! :)  Gregor and I happened to come across this today too (bug 843762).

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't track mousemove targets in Settings

Makes a small improvement.

It does have the side effect, however, of making the display and sound adjuster sliders buttery smooth.  /me shrugs

Comment 13

[Evelyn Hung]

I'm keep working on some small tweaks. These are not critical but really affect scrolling a little bit.
1. query database multiple times (will be fixed in bug 843762)
2. remove initial animation on toggles
3. media_storage.js did extra device state queries for the sub-menu but the sub-menu isn't exist in DOM (lazy-loading)
4. icc menu should be default visible to prevent extra reflow. (Most of the time, we have a SIM card in the phone, the menu has higher probability needs to be displayed.)
5. ... more, will file follow-up issues to refactor the bad smell in the code.

Will provide a patch tomorrow (TPE time)

Comment 14

[Evelyn Hung]

Sorry I was not able to work on the weekend... but I believe the quick fix from cjones do improve the scrolling speed.

kaze or Vivien, if you can take time to review the patch from cjones, it will be helpful. Thanks!

I will provide my patch on Monday for review. Sorry for the delay.

Comment 15

[Evelyn Hung]

Attachment: some tweaks to improve performance

1.remove initial animation on toggles
2.remove extra device states queries from media_storage.js
because these states are displayed only on the sub-menu which
is lazy-loaded
3.icc menu should be default visible to prevent extra reflow. (Most of the time, we have a SIM card in the phone, the menu has higher probability needs to be displayed.)
4. Don't track mousemove targets in Settings (patch from Chris Jones, attachment 716846 [details] [diff] [review])
5. Don't query settings database multiple times when initialing. (patch from Chris Jones on bug 843762 attachment 716711 [details] [diff] [review])

Comment 16

[Evelyn Hung]

Comment on attachment 716846 [details] [diff] [review]
Don't track mousemove targets in Settings

cancel the review request because I've included it in my patch which r=timdream

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> It seems like it might be a good idea to stop deoptimizing uniform-color
> thebeslayers when they force buffer alloc traffic.  But that's too risky to
> do here.

OK. Filed bug 845172.

Comment 18

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

Comment on attachment 717810 [details]
some tweaks to improve performance

looks good

Comment 19

[Evelyn Hung]

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

Comment 20

[Michael Vines [:m1] [:evilmachines]]

(Requesting uplift to v1.0.1, as this is one of the bugs needed to make v1.0.1 commercially viable.)

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

We need more information here on how successful this patch is at changing the perception of performance on settings app.  Can someone from QA test out a build with the fix and report back?

Comment 22

[David Scravaglieri [:scravag]]

Vivien, could you estimate the risk and the win of this patch please ?

Comment 23

[Mila Davydova]

When launching Settings for the first time and swiping down as soon as app appears, no delay in scrolling occurs and user is able to scroll in full speed.
Note: Airplain toggle is set to off by default
      GPS toggle switches from off to on, ass soon as app appears
Unagi Build ID 20130227070202
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/dbc39e13c7e2
Gaia: f254d5b2af84cb680e60c4b747658f4cdf25c2b2

Comment 24

[Alex Keybl [:akeybl]]

Sorry, meant to leave this on the tef nom list while we wait for a risk estimate.

Comment 25

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

(In reply to Alex Keybl [:akeybl] from comment #24)
> Sorry, meant to leave this on the tef nom list while we wait for a risk
> estimate.

What I wonder if how much of this patch do we need to have the scrolling win? I'm not sure all the patch is needed?

Comment 26

[Michael Vines [:m1] [:evilmachines]]

(jhford -- uplift to v1.0.1 please?)

Comment 27

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

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 e8cb49b8782d93a197a927b48cd4c374268f7a37
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 28

[Evelyn Hung]

Bug 796798 is one of my patch's dependency. There is another conflict after cherry-pick patch of bug 796798 (commit 3df466a3a11c898adc1874c59784ce83a3bab300). I've resolved it, how do I merge my commit to v1-train/v1.0.1? Do I have to submit a PR to these two branches?

Comment 29

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

(In reply to Evelyn Hung [:evelyn] from comment #28)
> Bug 796798 is one of my patch's dependency. There is another conflict after
> cherry-pick patch of bug 796798 (commit
> 3df466a3a11c898adc1874c59784ce83a3bab300). I've resolved it, how do I merge
> my commit to v1-train/v1.0.1? Do I have to submit a PR to these two branches?

Well, you could, and that'd work, but it's easier if you push your branch to your github account and I pull it in and do the merge, no reason to create two PRs.

Comment 30

[Evelyn Hung]

Okay, my github branch here: https://github.com/evelynhung/gaia/tree/v1-train
Thanks!

Comment 31

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

Squashed the commits and landed on v1-train as c5a8b6476f0dbc456061227a7801e56634683eb0

Comment 32

[Evelyn Hung]

For v1.0.1: https://github.com/evelynhung/gaia/commits/v1.0.1
Thank you so much.

Comment 33

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

Squashed commits and landed on v1.0.1 as 633e14d00c6d7a861415d73f4b25c3f1d43067fc