852489 - Miscellaneous optimizations to improve FPS scrolling B2G email app

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

I have some patches that together improve scrolling through the message list of the email app by a few FPS.

Comment 1

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

I did a bunch of small optimizations to reduce the calls to Layer::Mutated() and thence reduce IPC traffic to the compositor. I also did a couple of small optimizations to eliminate construction of unnecessary display items.

Comment 2

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

Attachment: Part 1: Don't create nsDisplayButtonForegrounds for buttons that aren't focused

Comment 3

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

Attachment: Part 2: Constify ns(Int)Region::ToString

Comment 4

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

Attachment: Part 3: Add logging to track addition of IPC edits and causes of layer mutation. Also avoid unnecessary mutate in ClearAnimations

Comment 5

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

Attachment: Part 4: Don't call RepositionChild if the layer is already in the right position

Alternatively we could do this check in the layer system somewhere. Seems like less code (and more efficient) to do it here.

Comment 6

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

Attachment: Part 5: Don't temporarily reset clip rect when reusing an existing layer

Comment 7

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

Part 5 seems too easy, please check it!

Comment 8

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

Attachment: Part 6: Don't temporarily reset clip when recycling Thebes/Image/ColorLayers

This also seemed suspiciously easy.

Comment 9

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

Attachment: Part 7: Remove Layer::IntersectClipRect because it's no longer used and it's prone to cause unnecessary Mutateds

Comment 10

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

Attachment: Part 8: Don't create nsDiplayBackgroundImages when there's no image

Comment 11

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

Attachment: Part 9: Avoid temporarily setting a layer's visible region to a non-final value

Comment 12

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

With these patches, scrolling the Gaia EMail app in the message pane without hitting the stuff at the beginning or end of the pane, the layer-tree updates that get send to the compositor are reduced to (in the common case):
-- a FrameMetrics update for the ScrollInfoLayer for the pane we're scrolling
-- VisibleRegion and Transform updates for the ThebesLayer with the scrolling content, and a paint for that layer
-- VisibleRegion update for the scrollbar that's moving, and a paint for that layer
That's about as good as we can expect to get. However, when layer attributes change we resend every attribute to the compositor, and it might make sense to make those updates more fine-grained in the future.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 726589 [details] [diff] [review]
Part 1: Don't create nsDisplayButtonForegrounds for buttons that aren't focused

r=mats

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726593 [details] [diff] [review]
Part 5: Don't temporarily reset clip rect when reusing an existing layer

Review of attachment 726593 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, can't find any callers that would have issues with it.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 726599 [details] [diff] [review]
Part 8: Don't create nsDiplayBackgroundImages when there's no image

Review of attachment 726599 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like we already have something like this on m-c.

Comment 16

[Matt Woodrow (:mattwoodrow)]

One final option for improving scrolling speed would be to look again at avoiding painting the scrollbar when it moves.

We could shift the scrollbar by using a transform, instead of actually moving. Unfortunately this is fairly difficult since it messes up reflow, maybe not a great idea for the b2g18 branch.

Alternatively we could try change the scrollbar css so that it's just a single background color or image, and gets optimized to a ColorLayer or ImageLayer. This would probably be a lot safer and should achieve the same result.

I believe Andreas measured a ~5fps gain from avoiding this extra painting work.

Comment 17

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> One final option for improving scrolling speed would be to look again at
> avoiding painting the scrollbar when it moves.

Yeah that does sound good.

Comment 18

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

(re: tef? -- any thoughts on the risk of an uplift to v1.0.1 once this stuff lands on m-c?  9 patches looks like plenty of room for regressions to hid in, so leaning towards a pass right now TBH.)

Comment 19

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Part 8: Don't create nsDiplayBackgroundImages when there's no image
> 
> Looks like we already have something like this on m-c.

Yes. We have part 2 on m-c already too.

Comment 20

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

https://tbpl.mozilla.org/?tree=Try&rev=a99333a5ee73

Comment 21

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

Attachment: Part 3.5: Ensure that Mutated is called for all layer attributes

I was getting test failures on try; this is the problem...

Comment 22

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

I've been working all week on getting these patches landed. The remaining problem right now is Android robocop-2 tests consistently failing in testSystemPages with the PaintExpecter timing out due to the paint not being fired (there is a known intermittent with this signature also). Bisection shows that part 5 is causing the bug. I suspect it's because we're not firing viewport changes due to navigation or something like that, but removing the PaintExpecter and polling the current URL to see when it changes didn't help.

I spent most of today getting the robocop tests set up locally to try to narrow down the problem, only to find that the test passes locally :-(.

Comment 23

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

Geoff: given we have an intermittent failure on this already (bug 797615) which it looks like my patches turn into a permanent failure, how would you feel about me disabling the relevant part of testSystemPages and landing my patches? I think it's up to someone who knows Fennec to figure out why the test fails on try.

Comment 24

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

Attachment: part 0: disable failing part of test

Comment 25

[Geoff Brown [:gbrown]]

Comment on attachment 730614 [details] [diff] [review]
part 0: disable failing part of test

Review of attachment 730614 [details] [diff] [review]:
-----------------------------------------------------------------

I think that's reasonable. Once this bug is resolved, I will follow-up to re-enable.

Sorry this has caused so much grief! 

(Note to self: During investigation, we noticed that loadUrl(about:config) does not wait for paint completion, so the following use of a PaintExpecter may complete on paint events from the previous load.)

Comment 26

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e930513d923
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa2721736af
https://hg.mozilla.org/integration/mozilla-inbound/rev/3658b65b0206
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b386ed03fb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a086dd52e5d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9916e4fb4eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/643b78450848
https://hg.mozilla.org/integration/mozilla-inbound/rev/c226b0174560
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad75012bfabf

Comment 27

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

I had to disable a bit more of the testSystemPages test --- the menu test also uses paintExpecter and was also failing.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e930513d923
https://hg.mozilla.org/mozilla-central/rev/2aa2721736af
https://hg.mozilla.org/mozilla-central/rev/3658b65b0206
https://hg.mozilla.org/mozilla-central/rev/8b386ed03fb5
https://hg.mozilla.org/mozilla-central/rev/a086dd52e5d4
https://hg.mozilla.org/mozilla-central/rev/d9916e4fb4eb
https://hg.mozilla.org/mozilla-central/rev/643b78450848
https://hg.mozilla.org/mozilla-central/rev/c226b0174560
https://hg.mozilla.org/mozilla-central/rev/ad75012bfabf

Comment 29

[Daniel Coloma:dcoloma]

Any feedback about the risk (see comment 18)?

Comment 30

[Geoff Brown [:gbrown]]

Suspected of causing a Talos regression on Android 4.0.4, tsvg_nochrome: see bug 856807.

Comment 31

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

I think the patches are individually low-risk. In the aggregate maybe they're not worth taking --- only a couple of FPS difference.

Comment 32

[Daniel Coloma:dcoloma]

based on comment 31 I would say we should not block. Michael, wdyt?

Comment 33

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

If it was my decision I would not land these in 1.0.1 at this point, but v1.1 for sure though.

Comment 34

[Daniel Coloma:dcoloma]

Agree, leo? is then

Comment 35

[Alex Keybl [:akeybl]]

Let's get an approval-mozilla-b2g18 nomination on this instead of blocking, and land as soon as possible.

Comment 36

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

I'd like to track down 856807 first.