Too many CanvasLayer on e.me results hurt panning

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

unspecified

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

Details

(Whiteboard: QARegressExclude)

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 786937 [details] [diff] [review]
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.
Created attachment 786984 [details] [diff] [review]
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.
Created attachment 787052 [details] [diff] [review]
patch.image.patch

Better patch from evyatar once this stuff has been realized.
Attachment #786937 - Attachment is obsolete: true
Attachment #787052 - Flags: review+
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.
blocking-b2g: --- → leo?
Attachment #786984 - Flags: review?(ran) → review+

Comment 5

5 years ago
(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).
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.
Created attachment 787492 [details] [diff] [review]
unmerge.while.editing.patch

Unmerge while editing since it is a bit too expensive in this case.
Attachment #787492 - Flags: review?(ran)

Comment 8

5 years ago
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.
blocking-b2g: leo? → -
Attachment #787492 - Flags: review?(ran) → review+
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...
blocking-b2g: - → leo?
(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.
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
blocking-b2g: leo? → leo+
This has landed on v1-train.
status-b2g18: --- → fixed
v1.1.0hd: a3612a020f3a85cc3cea0cd46714b44218b55f0c
v1.1.0hd: 7273a22f41c4e39912cb40c4f8ed2f14f16529ae
v1.1.0hd: f4231bc2ff87b3a43437b98b538fb01cf978d4b3
status-b2g-v1.1hd: --- → fixed

Updated

5 years ago
Whiteboard: QARegressExclude

Updated

5 years ago
Assignee: nobody → 21
You need to log in before you can comment on or make changes to this bug.