Closed Bug 1251937 Opened 5 years ago Closed 5 years ago

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


(Testing :: Talos, defect)

Version 3
Not set


(firefox46 fixed, firefox47 fixed, firefox48 fixed)

Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed


(Reporter: gkrizsanits, Assigned: kats)


(Depends on 1 open bug)



(1 file)

No longer blocks: apz-talos
Depends on: apz-talos
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.
Attached 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.

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+
Thanks! Filed bug 1254273 for it.
Excellent news. TResize also uses displayport suppression and thus might benefit in the same way?
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8727564 [details] [diff] [review]

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]

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.