Closed
Bug 1438551
Opened 6 years ago
Closed 6 years ago
Add option to use P-OMTP on windows when we are using Skia
Categories
(Core :: Graphics: Layers, enhancement, P3)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: rhunt, Assigned: rhunt)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
With bug 1438321 fixed, a try run that force enables skia and tiling and P-OMTP on windows is mostly green with some fuzzy reftests. [1] I think we should consider using tiling with P-OMTP when we decide to use skia on windows. I still need to do some performance testing to make sure this isn't a regression, though. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=380ae6f4d6a5d1ffda55a2199d602d815dd1ec41
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
Here's the results of a talos run comparing: 1. force enabled skia + rotated buffer + OMTP 2. force enabled skia + tiling + P-OMTP The base revision is [1], the new revision is [2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=89866af0dec8aa9486ec16a050068b8890a8b7fe&newProject=try&newRevision=c40c44f3ce756d1eb27a0c982c282b7e5ad23d33&framework=1
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #1) > Here's the results of a talos run comparing: > > 1. force enabled skia + rotated buffer + OMTP > 2. force enabled skia + tiling + P-OMTP > > The base revision is [1], the new revision is [2] > > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=89866af0dec8aa9486ec16a050068b88 > 90a8b7fe&newProject=try&newRevision=c40c44f3ce756d1eb27a0c982c282b7e5ad23d33& > framework=1 Some takeaways from the results. The rasterflood tests show the largest improvement of about ~30-70%. tsvgx improved by about ~10%. tsvgr_opacity may have also improved by a comparable amount. There are some page load and paint tests that showed slight improvements (1%-7%~). Some other tests may have regressed slightly, it's hard to tell. tscrollx regressed by about ~25% average. Digging into the subtests, 4 of the 12 didn't significantly regress at all, but the other 8 did by 30-50%. Curiously they all have very small background images tiled over the screen. We might be doing something dumb here. tsvg_static also looks like it regressed by about ~10%. Not sure why that test regressed but the other svg tests improved.
Assignee | ||
Comment 3•6 years ago
|
||
Here are two profiles I grabbed from running tscrollx locally on my machine: Force enabled skia + OMTP: https://perfht.ml/2BHpD2Y Force enabled skia + tiling + POMTP: https://perfht.ml/2omQNnv It seems like actual rasterization time is comparable between the two, but the main thread is blocked waiting on the compositor as it falls behind uploading textures for quite some time. Not sure why this is happening.
Assignee | ||
Comment 4•6 years ago
|
||
Turns out the reason we're spending so much time in uploading textures is because we're forcing a reupload of every tile every frame with tiling and AL [1]. With AL mLayerManager->Compositor() is nullptr, which causes us the TextureHost we're changing TextureSourceProviders, which causes us to reupload the whole TextureSource the next time it's used. It's not clear that line is even needed, because we set it properly to the layer manager's texture source compositor later in UseTile(). Fixing this resolved the tscrollx regression for me locally (tiling is actually slightly faster now). Testing on try now to see if there are any other regressions remaining. [1] https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/gfx/layers/composite/TiledContentHost.cpp#323
Assignee | ||
Comment 5•6 years ago
|
||
I've got a patch queue now that finally has positive talos results. [1] Cleaning up patches now for review. [1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bfb7ca5bc907&newProject=try&newRevision=153687217e2137b5a64e5ae1f6090207fe8f8cd3&framework=1
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfaa3a3195d98e4dedbacf00eb763bd900268a8f Looks green with two existing intermittents.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
Renaming the bug here, as I don't plan on enabling it for windows in the same push.
Summary: Enable P-OMTP on windows when we are using Skia → Add option to use P-OMTP on windows when we are using Skia
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8967021 [details] Bug 1438551 - When resizing single tile buffers be sure we don't mark the copied region as invalidated. https://reviewboard.mozilla.org/r/235700/#review241738
Attachment #8967021 -
Flags: review?(nical.bugzilla) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8967024 [details] Bug 1438551 - Add a pref for enabling tiles when we are using skia with parallel painting. https://reviewboard.mozilla.org/r/235706/#review241754
Attachment #8967024 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 14•6 years ago
|
||
Here are the talos and awsy runs: With holding on to the back buffer when we reuse the front buffer: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bfb7ca5bc907&newProject=try&newRevision=153687217e2137b5a64e5ae1f6090207fe8f8cd3&framework=1 With the current behavior of discarding the back buffer when we reuse the front buffer: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bfb7ca5bc907&newProject=try&newRevision=ae336f022a08f44772214d2f7609b1b6b2bded09&framework=1
Assignee | ||
Comment 15•6 years ago
|
||
Those talos runs are both this patch queue vs skia not tiling. Main changes if we continue not retaining the back buffer: 3.59% ts_paint regression 3.10% t_paint maybe regression 6.13% session_restore regression -5.43% cp_startup improvement
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8967529 [details] Bug 1438551 - Remove unused mPaintedRegion from TiledLayerBuffer. https://reviewboard.mozilla.org/r/236210/#review242176
Attachment #8967529 -
Flags: review?(nical.bugzilla) → review+
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8967022 [details] Bug 1438551 - When creating a back buffer, only paint in the visible rect. https://reviewboard.mozilla.org/r/235702/#review242182
Attachment #8967022 -
Flags: review?(nical.bugzilla) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8967023 [details] Bug 1438551 - Don't discard the back buffer when we reuse the front buffer. https://reviewboard.mozilla.org/r/235704/#review242532
Attachment #8967023 -
Flags: review?(nical.bugzilla) → review+
Comment 26•6 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c07a8364334 When resizing single tile buffers be sure we don't mark the copied region as invalidated. r=nical https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5ed3dd38b9 When creating a back buffer, only paint in the visible rect. r=nical https://hg.mozilla.org/integration/mozilla-inbound/rev/83a97d3d6b77 Don't discard the back buffer when we reuse the front buffer. r=nical https://hg.mozilla.org/integration/mozilla-inbound/rev/2b481be0024f Add a pref for enabling tiles when we are using skia with parallel painting. r=nical https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2919c16c8c Remove unused mPaintedRegion from TiledLayerBuffer. r=nical
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5c07a8364334 https://hg.mozilla.org/mozilla-central/rev/ab5ed3dd38b9 https://hg.mozilla.org/mozilla-central/rev/83a97d3d6b77 https://hg.mozilla.org/mozilla-central/rev/2b481be0024f https://hg.mozilla.org/mozilla-central/rev/6a2919c16c8c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•