Add option to use P-OMTP on windows when we are using Skia

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rhunt, Assigned: rhunt)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 attachments)

Assignee

Description

a year ago
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
Assignee

Comment 1

a year 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

a year 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

a year 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

a year 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

a year 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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

a year 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

a year 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

a year 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 15

a year 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

a year 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

a year 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

a year 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

a year 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
You need to log in before you can comment on or make changes to this bug.