Align to sub-tile areas rather than full tiles

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks 1 bug, {perf})

48 Branch
mozilla48
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

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.
Blocks: apz-talos
Keywords: perf
Whiteboard: [gfx-noted]
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
Posted patch PatchSplinter Review
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
Flags: needinfo?(bugmail.mozilla)
Looks like bug 1216924 was the culprit. I'll investigate and reland eventually.
Flags: needinfo?(bugmail.mozilla)
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 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e89d1b9a722
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Kats, should we uplift this APZ optimization to Aurora 47 in preparation for our e10s+APZ experiment on Beta 47?
Flags: needinfo?(bugmail.mozilla)
Yup, thanks for catching this!
No longer depends on: 1231396
Flags: needinfo?(bugmail.mozilla)
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 [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
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.