902465 - Too many CanvasLayer on e.me results hurt panning

Status: RESOLVED FIXED

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

Description

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

Attachment: shortcuts.img.wip.patch

Right now the e.me results are using canvas instead of DOM elements mostly for performance reasons while scrolling the results from top to bottom.

So each result on the screen is a canvas element.

Afaict each of those canvas results into a CanvasLayer that can not be merged into a bigger layer when scrolling from top to bottom or when panning between homescreen pages.

Replacing those canvas by img elements saves more than 5fps when panning. I have not measured while scrolling but it looks good too.

I would like to see that for 1.1 for even if it seems safe for Shortcuts I don't know the impact for Apps results.

Comment 1

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

Attachment: e.me.results.same.layer.patch

The previous converts canvas to img and this one force all imgs to be rendered into the same layer instead of having one layer per row that is created/destroyed based on some gecko heuristic.

Comment 2

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

Attachment: patch.image.patch

Better patch from evyatar once this stuff has been realized.

Comment 3

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

I think those 2 patches are harmless and improve the scrolling / panning experience of e.me.

I can live without it if this is really needed but that would be a shame. Those will land on master today and I recommend to land them on 1.1 if no regressions are found.

Comment 4

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

https://github.com/mozilla-b2g/gaia/commit/2d143dd34f66a148106d915b8afd5d4641d219c1
https://github.com/mozilla-b2g/gaia/commit/2db7fc9697115575b942c92de3d32906adbdc3ef

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #3)
> I think those 2 patches are harmless and improve the scrolling / panning
> experience of e.me.
> 
> I can live without it if this is really needed but that would be a shame.
> Those will land on master today and I recommend to land them on 1.1 if no
> regressions are found.

I think we should probably avoid this patch - feels like the risk/reward calculus doesn't really make sense here when the only issue is panning perf (not a regression from 1.0.1, I'm assuming).

Comment 6

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

Just to advocate for the patch- the issue is both panning performance and vertical scrolling, which we feel is crucial since shortcuts and apps are going to be in the user's face a lot of the time.

I agree the patch looks risky, since it touches 4 different files, but in essence there's no new logic introduced - only changing from canvas tags to img.

Comment 7

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

Attachment: unmerge.while.editing.patch

Unmerge while editing since it is a bit too expensive in this case.

Comment 8

[Alex Keybl [:akeybl]]

Is the performance a regression from 1.0.1 in any way? If not, this will land in 1.1. Please only renominate if this is perf regression instead of just a win.

Comment 9

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

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

Last part. I strongly suggest to rethink the leo? flag since this will be in the user face all the time and this is the version that we have given to QA folks...

Comment 10

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

(In reply to Alex Keybl [:akeybl] from comment #8)
> Is the performance a regression from 1.0.1 in any way? If not, this will
> land in 1.1. Please only renominate if this is perf regression instead of
> just a win.

I don't think you should consider regression as the only criteria here. A bug in a part of the UI never used does not matter, a bug in a part of the UI that is the first thing we shown to user matters a bit much.

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

Paris triage: leo+. the difference is quite big before and after the patch. Since this is now the center of the homescreen, user experience on this should weight more

Comment 12

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

This has landed on v1-train.

Comment 13

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

v1.1.0hd: a3612a020f3a85cc3cea0cd46714b44218b55f0c
v1.1.0hd: 7273a22f41c4e39912cb40c4f8ed2f14f16529ae
v1.1.0hd: f4231bc2ff87b3a43437b98b538fb01cf978d4b3