Closed
Bug 1262807
Opened 9 years ago
Closed 9 years ago
Lots of repainting while scrolling around in Fennec
Categories
(Core :: Layout, defect, P1)
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)
58 bytes,
text/x-review-board-request
|
mstange
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details |
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.
Assignee | ||
Updated•9 years ago
|
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)
Assignee | ||
Comment 2•9 years ago
|
||
mxr is down, what URL are you testing this on?
Flags: needinfo?(snorp)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
status-firefox49:
--- → affected
status-firefox50:
--- → affected
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59508/
Attachment #8763238 -
Flags: review?(mstange)
Attachment #8763239 -
Flags: review?(mstange)
Assignee | ||
Comment 8•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59510/
Assignee | ||
Comment 9•9 years ago
|
||
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•9 years ago
|
||
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 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
Try push looks good, going to land these.
Comment 15•9 years ago
|
||
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
Comment 16•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0468fca8fde3
https://hg.mozilla.org/mozilla-central/rev/a1ab2e55d129
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 18•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Attachment #8763239 -
Flags: approval-mozilla-beta?
Attachment #8763239 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
status-firefox47:
--- → unaffected
Keywords: regression
Assignee | ||
Updated•9 years ago
|
Priority: -- → P1
Comment 19•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bugmail.mozilla)
Comment 22•9 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 23•9 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/b45533da98aa
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/a6696f118cb2
Flags: needinfo?(bugmail.mozilla)
Updated•9 years ago
|
Attachment #8763239 -
Flags: approval-mozilla-beta?
Attachment #8763239 -
Flags: approval-mozilla-beta+
Attachment #8763239 -
Flags: approval-mozilla-aurora?
Attachment #8763239 -
Flags: approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•