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

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Gabor Krizsanits (INACTIVE)]

Testing with APZ disabled: https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=ad4d966f2367&newProject=try&newRevision=a2937311d493&framework=1

Comment 1

[Matt Woodrow (:mattwoodrow)]

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

[Benoit Girard (:BenWa)]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 5

[Kartikaya Gupta (email:[e-mail])]

We could even go further and drop tile alignment completely for displayport suppression cases.

Comment 6

[Benoit Girard (:BenWa)]

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?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 8

[Benoit Girard (:BenWa)]

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Hm, good point. I'll do that in a different bug then. Mind r+'ing this one?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Thanks! Filed bug 1254273 for it.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/216eab5f3394

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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

[Benoit Girard (:BenWa)]

Excellent news. TResize also uses displayport suppression and thus might benefit in the same way?

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/216eab5f3394

Comment 15

[Kartikaya Gupta (email:[e-mail])]

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

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8727564 [details] [diff] [review]
Patch

Perf improvement in APZ, taking it.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdaa88cdffe3

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9690d9acba01