APZ causing ~40ms of > regression on OS X for TPS

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gkrizsanits, Assigned: kats)

Tracking

(Depends on 1 bug)

Version 3
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

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.
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.
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.
Posted patch PatchSplinter Review
Assignee: nobody → bugmail.mozilla
Attachment #8727564 - Flags: review?(bgirard)
We could even go further and drop tile alignment completely for displayport suppression cases.
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)
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)
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.
Hm, good point. I'll do that in a different bug then. Mind r+'ing this one?
Attachment #8727564 - Flags: review?(bgirard) → review+
Excellent news. TResize also uses displayport suppression and thus might benefit in the same way?
https://hg.mozilla.org/mozilla-central/rev/216eab5f3394
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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+
You need to log in before you can comment on or make changes to this bug.