Closed Bug 1254273 Opened 5 years ago Closed 5 years ago
Align to sub-tile areas rather than full tiles
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.
This may or may not actually help talos tests, but marking it as blocking apz-talos anyway just in case.
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
Assignee: nobody → bugmail.mozilla
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
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?
Attachment #8729549 - Flags: review?(bgirard)
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.
Attachment #8729549 - Flags: review?(bgirard) → review+
(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.
Yea, that's what I meant. Your approach also seems reasonable, and better than what we have now, so let's try it.
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
Looks like bug 1216924 was the culprit. I'll investigate and reland eventually.
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.
Kats, should we uplift this APZ optimization to Aurora 47 in preparation for our e10s+APZ experiment on Beta 47?
Yup, thanks for catching this!
5 years ago
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Version: unspecified → 48 Branch
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  for memory improvement [Risks and why]: pretty low risk, should only affect OS X [String/UUID change made/needed]: none  https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=%5Bmozilla-inbound,ce9560a05bd11e46e1898331e206d3b393b2cc6f,1%5D&zoom=1457844016113.402,1458095408412.371,303478252.0238904,391884049.1253397
Attachment #8729549 - Flags: approval-mozilla-aurora?
Comment on attachment 8729549 [details] [diff] [review] Patch Perf/mem improvements in APZ, Aurora47+
Attachment #8729549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.