842213 - Homescreen: Poor FPS / jank when panning in/out of everything.me

Status: VERIFIED FIXED

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

Description

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

STR:
1) Goto homescreen
2) Pan or swipe left to expose e.me

Observe FPS is quite low and often pretty bad jank occurs.

Comment 1

[Cristian Rodriguez (:crdlc)]

The problem is here [1]. If this code is changed for an image (for example [2]) the FPS increases 15~20 units

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything.me/modules/Apps/Apps.js#L579
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/images/default.png

Comment 2

[Evyatar 'Tron' Amitay (everything.me)]

We're actually right in the middle of working on it! Currently each shortcut is comprised of 3 SPANs with background image, border radius and box shadow which makes it very slow.

I'm working on redrawing each shortcut as a canvas, which is pretty much like saying it's a single image- just like on the homescreen grid. This improves performance for both panning in and out, and scrolling of the list itself.

So expect it soon!

Comment 3

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

Hey Evyatar,
Any chance of getting a WIP patch for this today?
Thanks!
Michael

Comment 4

[Evyatar 'Tron' Amitay (everything.me)]

Attachment: Patch - redirect to github PR

Comment 5

[Evyatar 'Tron' Amitay (everything.me)]

Did you one better- this is the final (before review) patch.
Shortcuts are replaced entirely with canvas elements.

Comment 6

[Evyatar 'Tron' Amitay (everything.me)]

Please note that part of the optimization was adding the text to the icon canvas- so each shortcut is a single canvas element now.
However, this means that when removing (long tap on a shortcut)- the animation is on the text as well. Previously only the icon pulsated.

We tried an approach of separating into two canvases- one for the icon and one for the text- and it affected the performance (around 6 FPS).

So if this feature (pulsating only image and not text) is important, we can change it- just note that it will affect performance.

Comment 7

[Cristian Rodriguez (:crdlc)]

ok, testing, thanks

Comment 8

[Cristian Rodriguez (:crdlc)]

one question, although categories names are rendered in canvas we translate them, is it?

Comment 9

[Evyatar 'Tron' Amitay (everything.me)]

yeap everything's translated using l10n. switching languages also redraws the canvases with the new translated name.

Comment 10

[Cristian Rodriguez (:crdlc)]

ok thanks

Comment 11

[Cristian Rodriguez (:crdlc)]

Comment on attachment 716492 [details]
Patch - redirect to github PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: poor fps going to ev.me
Testing completed: yes
Risk to taking this patch (and alternatives if risky): medium
String or UUID changes made by this patch:

Comment 12

[Evyatar 'Tron' Amitay (everything.me)]

cristian- I want to apply the same fix to the list of apps as well. currently it's just on the shortcuts. can we wait with the approval a bit?

Comment 13

[Cristian Rodriguez (:crdlc)]

That is other bug. It is based on poor fps in panning. The other is related to improve fps in scroll in categories and searches

Comment 14

[Cristian Rodriguez (:crdlc)]

feel free open other bug, so it is easier nominating this one to v1

Comment 15

[Cristian Rodriguez (:crdlc)]

https://github.com/mozilla-b2g/gaia/commit/14e0e00a46277e2560b82025d0b90cda900de130

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to crdlc from comment #11)

> Risk to taking this patch (and alternatives if risky): medium

Can you expand on the potential risk here?  Has there been testing of this on nightlies?

Comment 17

[Cristian Rodriguez (:crdlc)]

well, basically it paints the category icons with other mechanism better for FPS. I tested as reviewer on my phone and works fine. But could be dangerous if icons don't appear :) Please Ran could you give you feedback? I am only the reviewer.

Comment 18

[Ran Ben Aharon [:ranbena] EverythingMe]

I would say the risk is low. Instead of displaying stacked elements with backgrounds, we stack images in a canvas in a similar manner.

If you want to be confident about this change, perhaps give it a few days (now that it's merged in master) and see if we get any feedback.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 716492 [details]
Patch - redirect to github PR

Has been on nightlies for several days now, approving for v1-train uplift.

Comment 20

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

v1-train@b85c566

Comment 21

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

Requesting uplift to 1.0.1.  This patch was a part of the MWC build and makes a huge difference to the perceived UX of e.me

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Given that this has only been on v1-train for one day, and it's not a critical functionality issue - the size and scope of this change makes it not a viable uplift for v1.0.1 a day before CS and we'll have to wait for this to be in v1.1

Comment 23

[Andreas Gal :gal]

This was +-ed as part of OEM-requested performance work.

Comment 24

[Natalya Kot [:nkot]]

fix verification,

tested on trunk, 

Build ID: 2013-03-04-031028
Gecko  http://hg.mozilla.org/mozilla-central/rev/86c98c4d36da
Gaia   905cc8802794db12eddb0f94dca9dc5d0d3f4e67

- looks good, swiping to ev.me shows FPS above 50... 53-56 
(max seen - 59, min - 42)

Comment 25

[Evyatar 'Tron' Amitay (everything.me)]

I'm not sure what the testing protocol for this is, but please make sure to include this bug as well https://bugzilla.mozilla.org/show_bug.cgi?id=844565

they're pretty much the same change only to different areas of the app, so as far as I know they're supposed to go together for the performance sprint.

Comment 26

[Daniel Coloma:dcoloma]

John, was this uplifted to v1.0.1?

Comment 27

[Daniel Coloma:dcoloma]

This is not uplifted to v1.0.1 and it is tef+, John, can you please uplift?

Comment 28

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

(In reply to Daniel Coloma:dcoloma from comment #27)
> This is not uplifted to v1.0.1 and it is tef+, John, can you please uplift?

This does not apply cleanly to v1.0.1.  A merge conflict can be attempted with

  cd gaia
  git checkout v1.0.1
  git cherry-pick -x -m1 14e0e00a46277e2560b82025d0b90cda900de130
  <RESOLVE MERGE CONFLICTS>
  git add apps/homescreen/everything.me/js/helpers/Utils.js
  git add apps/homescreen/everything.me/modules/Shortcuts/Shortcuts.css
  git commit

Comment 29

[Evyatar 'Tron' Amitay (everything.me)]

done, please just make sure I didn't mess it up (repo-wise).
https://github.com/mozilla-b2g/gaia/commit/2d048a9bdae54e4ec7d48326c2130591c8b869b6

Comment 30

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

(In reply to Evyatar 'Tron' Amitay (everything.me) from comment #29)
> done, please just make sure I didn't mess it up (repo-wise).
> https://github.com/mozilla-b2g/gaia/commit/
> 2d048a9bdae54e4ec7d48326c2130591c8b869b6

That looks like a sane merge, but with the proviso that I don't know the first thing about that code.

Comment 31

[Evyatar 'Tron' Amitay (everything.me)]

that's ok, just wanted to make sure I didn't mess the repo's integrity or anything. thanks.