Closed
Bug 1251937
Opened 5 years ago
Closed 5 years ago
APZ causing ~40ms of > regression on OS X for TPS
Categories
(Testing :: Talos, defect)
Tracking
(firefox46 fixed, firefox47 fixed, firefox48 fixed)
RESOLVED
FIXED
mozilla48
People
(Reporter: gkrizsanits, Assigned: kats)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
1.38 KB,
patch
|
BenWa
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Testing with APZ disabled: https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=ad4d966f2367&newProject=try&newRevision=a2937311d493&framework=1
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 1•5 years ago
|
||
It looks like the extra time is being spent in texture upload. If we're painting a display port (the default with e10s), then we'll be painting a bigger area, and will spend longer uploading it. This extra first paint cost gives us wins when scrolling. cc'ing BenWa, since he's looked at this performance trade-off before, I don't remember where it ended up.
Comment 2•5 years ago
|
||
I landed a suppress displayport feature that should be activated on the first paint on a tab switch like TPS. If that paint has a display port then we should fix that.
Assignee | ||
Comment 3•5 years ago
|
||
The displayport suppression is working, but on OSX we still have tile alignment happening, so we're still painting a larger area. When I use force the displayport to be 128-aligned instead of tile-aligned during displayport suppression, the score on OSX improves significantly.
Assignee | ||
Comment 4•5 years ago
|
||
Assignee: nobody → bugmail.mozilla
Attachment #8727564 -
Flags: review?(bgirard)
Assignee | ||
Comment 5•5 years ago
|
||
We could even go further and drop tile alignment completely for displayport suppression cases.
Comment 6•5 years ago
|
||
As we discussed in person I think we should probably start aligning to the display port into an integer divisor of the display port regardless of suppression. Something like this: alignment = Math.min(256, tileSize). If it's a win with the displayport suppressed, it's probably an overall win. And overall IMO this would lead to simpler code. However in the spirit of being pragmatic I think this patch is fine if we want to land this ASAP while we investigate the above. Thoughts?
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 7•5 years ago
|
||
That sounds fine to me, I can make the change. However I'd prefer to use 128 instead of 256 (or maybe make it preffable) - any thoughts? I say 128 because that's what the existing virtual tiling code does and if we bump it to 256 it's likely going to make talos worse. That being said we could bump it to 256 and use 128 (or smaller) with displayport suppression. Thoughts?
Flags: needinfo?(bugmail.mozilla)
Comment 8•5 years ago
|
||
I'm fine with 128. I originally just went with 128 because I figured going from 1(implicit)->128 alignment was less drastic but I have no idea if 256 would of been a better choice. Note that what I'm suggesting might be harder to stick the landing compared to your patch above. My suggestion has the added risk that we might be doing worse case 1024/128=8 more full 1024x1024 upload per tile during scrolling where as the attached patch would have no effect for non-suppressed scrolling.
Assignee | ||
Comment 9•5 years ago
|
||
Hm, good point. I'll do that in a different bug then. Mind r+'ing this one?
Updated•5 years ago
|
Attachment #8727564 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 10•5 years ago
|
||
Thanks! Filed bug 1254273 for it.
Assignee | ||
Updated•5 years ago
|
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Assignee | ||
Updated•5 years ago
|
status-firefox48:
--- → affected
Assignee | ||
Comment 12•5 years ago
|
||
This definitely helps tps on OS X: https://treeherder.mozilla.org/perf.html#/graphs?series=[mozilla-inbound,ba8ccda021618c02de072c68b0b56ba251f42abd,1]&zoom=1457327398787.5354,1457418808779.0369,100,145.9851289770417
Comment 13•5 years ago
|
||
Excellent news. TResize also uses displayport suppression and thus might benefit in the same way?
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/216eab5f3394
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 15•5 years ago
|
||
Comment on attachment 8727564 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: tiling on OS X, APZ [User impact if declined]: with APZ enabled tab switch/resize operations on tiling platforms (i.e. OS X) are a little slower than they need to be [Describe test coverage new/current, TreeHerder]: covered by talos [Risks and why]: low risk, small change [String/UUID change made/needed]: none
Attachment #8727564 -
Flags: approval-mozilla-beta?
Attachment #8727564 -
Flags: approval-mozilla-aurora?
Comment on attachment 8727564 [details] [diff] [review] Patch Perf improvement in APZ, taking it.
Attachment #8727564 -
Flags: approval-mozilla-beta?
Attachment #8727564 -
Flags: approval-mozilla-beta+
Attachment #8727564 -
Flags: approval-mozilla-aurora?
Attachment #8727564 -
Flags: approval-mozilla-aurora+
Comment 17•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/fdaa88cdffe3
Comment 18•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9690d9acba01
You need to log in
before you can comment on or make changes to this bug.
Description
•