Closed Bug 852489 Opened 7 years ago Closed 7 years ago

Miscellaneous optimizations to improve FPS scrolling B2G email app

Categories

(Core :: Layout, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
blocking-b2g -
Tracking Status
b2g18 + ---

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(11 files)

1.70 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.69 KB, patch
mats
: review+
Details | Diff | Splinter Review
16.74 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.01 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.89 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.30 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.00 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.05 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
15.92 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.24 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.84 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
I have some patches that together improve scrolling through the message list of the email app by a few FPS.
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.
Alternatively we could do this check in the layer system somewhere. Seems like less code (and more efficient) to do it here.
Attachment #726592 - Flags: review?(matt.woodrow)
Part 5 seems too easy, please check it!
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 on attachment 726589 [details] [diff] [review]
Part 1: Don't create nsDisplayButtonForegrounds for buttons that aren't focused

r=mats
Attachment #726589 - Flags: review?(matspal) → review+
Attachment #726590 - Flags: review?(matspal) → review+
Attachment #726591 - Flags: review?(matt.woodrow) → review+
Attachment #726592 - Flags: review?(matt.woodrow) → review+
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.
Attachment #726593 - Flags: review?(matt.woodrow) → review+
Attachment #726596 - Flags: review?(matt.woodrow) → review+
Attachment #726598 - Flags: review?(matt.woodrow) → review+
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.
Attachment #726599 - Flags: review?(matt.woodrow) → review+
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.
Attachment #726604 - Flags: review?(matt.woodrow) → review+
(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.
blocking-b2g: --- → tef?
(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.)
(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.
I was getting test failures on try; this is the problem...
Attachment #729382 - Flags: review?(matt.woodrow)
Attachment #729382 - Flags: review?(matt.woodrow) → review+
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 :-(.
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.
Flags: needinfo?(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.)
Attachment #730614 - Flags: review?(gbrown) → review+
Flags: needinfo?(gbrown)
I had to disable a bit more of the testSystemPages test --- the menu test also uses paintExpecter and was also failing.
Any feedback about the risk (see comment 18)?
Flags: needinfo?(roc)
Suspected of causing a Talos regression on Android 4.0.4, tsvg_nochrome: see bug 856807.
I think the patches are individually low-risk. In the aggregate maybe they're not worth taking --- only a couple of FPS difference.
Flags: needinfo?(roc)
based on comment 31 I would say we should not block. Michael, wdyt?
Flags: needinfo?(mvines)
If it was my decision I would not land these in 1.0.1 at this point, but v1.1 for sure though.
Flags: needinfo?(mvines)
Agree, leo? is then
blocking-b2g: tef? → leo?
Let's get an approval-mozilla-b2g18 nomination on this instead of blocking, and land as soon as possible.
blocking-b2g: leo? → -
tracking-b2g18: --- → +
I'd like to track down 856807 first.
Depends on: 947467
You need to log in before you can comment on or make changes to this bug.