1254273 - Align to sub-tile areas rather than full tiles

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

On OSX tile sizes are 1024 pixels, so when we align the displayport to tiles it can end up significantly larger than it was before. The reason we align to tiles at all is to avoid having to upload a whole tile over and over if the displayport moves a little bit. However, for really large tile sizes it might make sense to align to a half-tile or quarter-tile.

BenWa and I discussed this (see https://bugzilla.mozilla.org/show_bug.cgi?id=1251937#c6 and onwards) and it's something we can try, but we need to figure out a way to measure the impact on uploads. Bug 1231396 would probably help with this.

Comment 1

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

This may or may not actually help talos tests, but marking it as blocking apz-talos anyway just in case.

Comment 2

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

Pushed to try on top of my patch for bug 1216924, let's see what talos says...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b02802d640

Comment 3

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

It seems to improve tp5o (page load time) and tp5o Main_RSS (memory usage) without regressing tp5o_scroll or tscrollx significantly. Seems like a win in talos, I'm not sure how much it maps to real-world situations though.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=dd1abe874252&newProject=try&newRevision=13b02802d640&framework=1

Comment 4

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

Attachment: Patch

I'm not entirely sure how to measure the impact of this in the wild. If there's a regression, do we have telemetry probes that would pick that up?

Comment 5

[Benoit Girard (:BenWa)]

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

I find it a bit odd that we would use 256 here as a magic number and 128 below, but it's not quite the same thing so I think it's fine.

FWIW: I intuitively would of done 'gfxPlatform::GetPlatform()->GetTileWidth() / 2' if the tiles are 512 or larger. I have reason/data to think it would be better or worse than what you're doing here.

I'm not able to load the telemetry stats for 'PAINT_RASTERIZE_TIME'. It's not the right metric to measure this anyways since it will likely reduce it. I'm not aware of a good end-to-end telemtry state that would accurately measure a change like this.

Comment 6

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

(In reply to Benoit Girard (:BenWa) from comment #5)
> FWIW: I intuitively would of done
> 'gfxPlatform::GetPlatform()->GetTileWidth() / 2' if the tiles are 512 or
> larger. I have reason/data to think it would be better or worse than what
> you're doing here.

I'm assuming you meant "no reason/data" :) And yeah I was kind of considering something like that as well but in my mind I was thinking that the primary goal is to avoid the displayport from expanding too much and that's independent of what the actual tile size is. If we discover that this regresses stuff too much we can adjust it.

Comment 7

[Benoit Girard (:BenWa)]

Yea, that's what I meant. Your approach also seems reasonable, and better than what we have now, so let's try it.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/622e87797446

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1254273 and bug 1216924 for M(4) failures.

Backouts:
Bug 1216924 https://hg.mozilla.org/integration/mozilla-inbound/rev/29f0eadfe982
Bug 1254273 https://hg.mozilla.org/integration/mozilla-inbound/rev/da961dd18d91

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=622e87797446
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23573589&repo=mozilla-inbound

11:20:46     INFO -  1621 INFO TEST-START | gfx/layers/apz/test/mochitest/test_bug982141.html
11:20:46     INFO -  TEST-INFO | started process screencapture
11:20:46     INFO -  TEST-INFO | screencapture: exit 0
11:20:46     INFO -  1622 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | expected at least one paint in compositor test data
11:20:46     INFO -  1623 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | found the RCD node
11:20:46     INFO -  1624 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | expected a single child APZC
11:20:46     INFO -  1625 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | expected a content paint with sequence number2
11:20:46     INFO -  1626 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | expected scroll frame data for scroll id 9
11:20:46     INFO -  1627 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | expected a displayport for scroll id 9
11:20:46     INFO -  1628 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | expected displayport string of form (x,y,w,h)
11:20:46     INFO -  1629 INFO TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_bug982141.html | expected a displayport at least as large as the scrollable element, got (0,0,36,36)
11:20:46     INFO -      testBug982141@gfx/layers/apz/test/mochitest/helper_bug982141.html:85:7
11:20:46     INFO -      afterPaint@gfx/layers/apz/test/mochitest/helper_bug982141.html:31:7
11:20:46     INFO -      EventListener.handleEvent*window.onload@gfx/layers/apz/test/mochitest/helper_bug982141.html:19:9
11:20:46     INFO -      EventHandlerNonNull*@gfx/layers/apz/test/mochitest/helper_bug982141.html:18:5

Comment 10

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

Looks like bug 1216924 was the culprit. I'll investigate and reland eventually.

Comment 11

[Benoit Girard (:BenWa)]

Yes, my money is also on bug 1216924. I remember that without the rounding-out in some cases part of the page wouldn't be painted and would lead to perma-checkerboard. I didn't fully understand why and it was something that I was wanting to look at before coming back to that bug. Just FWIW, might not be related.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e89d1b9a722

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e89d1b9a722

Comment 14

[Chris Peterson [:cpeterson]]

Kats, should we uplift this APZ optimization to Aurora 47 in preparation for our e10s+APZ experiment on Beta 47?

Comment 15

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

Yup, thanks for catching this!

Comment 16

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

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

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: Scrolling performance is a little worse, and memory usage is significantly worse
[Describe test coverage new/current, TreeHerder]: There are existing tests to cover correctness. See [1] for memory improvement
[Risks and why]: pretty low risk, should only affect OS X
[String/UUID change made/needed]: none

[1] https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=%5Bmozilla-inbound,ce9560a05bd11e46e1898331e206d3b393b2cc6f,1%5D&zoom=1457844016113.402,1458095408412.371,303478252.0238904,391884049.1253397

Comment 17

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

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

Perf/mem improvements in APZ, Aurora47+

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/71affd40d13f