Closed Bug 1262807 Opened 4 years ago Closed 4 years ago

Lots of repainting while scrolling around in Fennec

Categories

(Core :: Layout, defect, P1)

48 Branch
Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: kats, Assigned: kats)

References

()

Details

(Keywords: perf, regression)

Attachments

(2 files)

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.
Blocks: apz-fennec
No longer blocks: fennec-aboard-apz
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?
Flags: needinfo?(milan)
mxr is down, what URL are you testing this on?
Flags: needinfo?(snorp)
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.
Flags: needinfo?(snorp)
I can repro this consistently enough to fix it, I think. It's just a matter of trying out a few things.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(milan)
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
Blocks: 1236043
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.
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 on attachment 8763238 [details]
Bug 1262807 - Add logging support for TiledRegion classes.

https://reviewboard.mozilla.org/r/59508/#review56638
Attachment #8763238 - Flags: review?(mstange) → review+
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 ||.
Attachment #8763239 - Flags: review?(mstange) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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
Try push looks good, going to land these.
Pushed by kgupta@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/0468fca8fde3
https://hg.mozilla.org/mozilla-central/rev/a1ab2e55d129
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
No longer blocks: 1279300
Duplicate of this bug: 1279300
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
Attachment #8763238 - Flags: approval-mozilla-beta?
Attachment #8763238 - Flags: approval-mozilla-aurora?
Attachment #8763239 - Flags: approval-mozilla-beta?
Attachment #8763239 - Flags: approval-mozilla-aurora?
Priority: -- → P1
Comment on attachment 8763238 [details]
Bug 1262807 - Add logging support for TiledRegion classes.

Recent regression, taking it.
Should be in 48 beta 3
Attachment #8763238 - Flags: approval-mozilla-beta?
Attachment #8763238 - Flags: approval-mozilla-beta+
Attachment #8763238 - Flags: approval-mozilla-aurora?
Attachment #8763238 - Flags: approval-mozilla-aurora+
Hitting conflicts uplifting to beta because bug 1277862 isn't there.
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla)
Depends on: 1282060
No longer depends on: 1282060
Attachment #8763239 - Flags: approval-mozilla-beta?
Attachment #8763239 - Flags: approval-mozilla-beta+
Attachment #8763239 - Flags: approval-mozilla-aurora?
Attachment #8763239 - Flags: approval-mozilla-aurora+
Depends on: 1284940
Duplicate of this bug: 1274126
Blocks: 1274126
You need to log in before you can comment on or make changes to this bug.