1262807 - Lots of repainting while scrolling around in Fennec

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

Fennec Nightly, load http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js and scroll around with paint flashing turned on. There are lots of full-screen repaints which we can probably avoid because nothing is changing.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I've found that disabling low-res tiling seems to fix it. Some sort of regression with the paint invalidation there I guess? Milan, can you get an assignee?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

mxr is down, what URL are you testing this on?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

We were looking at this this morning. I could repro on my Nexus 4 but then I changed the displayport multipliers so that the stationary ones matched the skate ones, and that fixed it for me. It didn't fix it for snorp. It might be device differences; he was seeing it on a Nexus 6P and I couldn't see it on a Nexus 4 or Z3C.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I can repro this consistently enough to fix it, I think. It's just a matter of trying out a few things.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Chased down the bug, it seems to be an oversight in bug 1236043. The TiledRegion class added in that bug had a hard limit [1] beyond which the region effectively got simplified down to one rect. In our case, the low-res displayport was getting an invalid region that contained stuff at the very top and the very bottom, violating that limit, and therefore invalidating everything in between as well.

[1] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/gfx/src/TiledRegion.h#167

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Since this bug was filed before bug 1236043 landed, it may have originally been caused by something else. However, I'm ok with hijacking it for this fix :)

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc383567f623

I also suspect it will fix or at least help with the other heavy-checkerboard issues reported, marking those as depending on this as well.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1262807 - Add logging support for TiledRegion classes.

Review commit: https://reviewboard.mozilla.org/r/59508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59508/

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1262807 - Change the heuristic at which the TiledRegion collapses down to a single rect.

Review commit: https://reviewboard.mozilla.org/r/59510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59510/

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Ugh, the try push is giving crashes that to me don't make sense. I'll have to repro this on Linux (probably on Monday) to debug it.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8763238 [details]
Bug 1262807 - Add logging support for TiledRegion classes.

https://reviewboard.mozilla.org/r/59508/#review56638

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8763239 [details]
Bug 1262807 - Change the heuristic at which the TiledRegion collapses down to a single rect.

https://reviewboard.mozilla.org/r/59510/#review56640

Actually I think this doesn't need any additional comments.

::: gfx/src/TiledRegion.cpp:286
(Diff revision 1)
>    // of aRect with that tile to mRects, respecting the order of mRects.
>    // For each tile that already has a rectangle, we need to enlarge that
>    // existing rectangle to include the intersection of aRect with the tile.
>    return ProcessIntersectedTiles(aRect, mRects,
>      [&aRect](nsTArray<pixman_box32_t>& rects, size_t& rectIndex, TileRange emptyTiles) {
> -      if (!rects.InsertElementsAt(rectIndex, emptyTiles.Length(), fallible)) {
> +      if (rects.Length() >= kMaxTiles || !rects.InsertElementsAt(rectIndex, emptyTiles.Length(), fallible)) {

Let's add a line break after the ||.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Ugh, the try push is giving crashes that to me don't make sense. I'll have
> to repro this on Linux (probably on Monday) to debug it.

I can't repro it locally, possibly because I have linux64 build and based on the try push the failure is only on linux32. I've requested a loaner, and will try to repro in a local android build.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

I reproduced it in the Fennec emulator. The problem was that the emptyTiles.Length() can be massive; on one reftest it was upwards of 4 million. My guard only checked rects.Length() when it should have checked rects.Length() + emptyTiles.Length(), so that we don't ever even try to create an array that long.

New try push with that fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c7598a7a9f5

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Try push looks good, going to land these.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0468fca8fde3
Add logging support for TiledRegion classes. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ab2e55d129
Change the heuristic at which the TiledRegion collapses down to a single rect. r=mstange

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0468fca8fde3
https://hg.mozilla.org/mozilla-central/rev/a1ab2e55d129

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8763238 [details]
Bug 1262807 - Add logging support for TiledRegion classes.

Approval Request Comment
[Feature/regressing bug #]: bug 1236043
[User impact if declined]: severely degraded performance while scrolling with APZ (see bug 1279299, bug 1279433, bug 1279300)
[Describe test coverage new/current, TreeHerder]: the TiledRegion code is tested by gtests, but the fennec regression is hard to automate
[Risks and why]: low risk, the code is well understood. it was an oversight in the algorithm used in TiledRegion
[String/UUID change made/needed]: none

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8763238 [details]
Bug 1262807 - Add logging support for TiledRegion classes.

Recent regression, taking it.
Should be in 48 beta 3

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hitting conflicts uplifting to beta because bug 1277862 isn't there.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

The only conflicts I got was a #include line. There shouldn't be any relationship to bug 1277862. I can land it on aurora and beta once the trees reopen, and assuming Sylvestre +'s the second patch which he missed. Leaving ni on me for now.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35a893677058
https://hg.mozilla.org/releases/mozilla-aurora/rev/f74a3bf1496e

Comment 23

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b45533da98aa
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/a6696f118cb2