Closed Bug 1425056 Opened 2 years ago Closed 2 years ago

Implement parallel painting with OMTP tiled

Categories

(Core :: Graphics: Layers, enhancement, P3)

Unspecified
macOS
enhancement

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox59 --- verified

People

(Reporter: rhunt, Assigned: rhunt)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(6 files)

I've got a prototype of this working now and passing try.

Essentially the patch series will create capture draw targets for each tile, spin up a worker thread pool, and dispatch each tile and its capture individually to the thread pool. Talos results look okay, it might need some fine tuning.
Comment on attachment 8938182 [details]
Report a DrawTargetTiled as a capture if it is made of captures (bug 1425056, )

https://reviewboard.mozilla.org/r/208906/#review215282
Attachment #8938182 - Flags: review?(bas) → review+
Comment on attachment 8938183 [details]
Make a CaptureTiledPaintState for each tile (bug 1425056, )

https://reviewboard.mozilla.org/r/208908/#review215284

I'm a little concerned about how this will perform in machines with only two cores (i.e. where we won't win much in terms of perf from parallelism, but we'll have more capture overhead). But for now I don't think that'll be too important since this is OS X only for the moment.
Attachment #8938183 - Flags: review?(bas) → review+
Comment on attachment 8938184 [details]
Create a PaintWorker thread pool and dispatch tiles to it (bug 1425056, )

https://reviewboard.mozilla.org/r/208910/#review215286

::: gfx/layers/PaintThread.cpp:141
(Diff revision 1)
> +  // If not manually specified, default to (cpuCores * 3) / 4
> +  if (workerCount < 1) {
> +    workerCount = std::max((cpuCores * 3) / 4, 1);
> +  }
> +
> +  // Sanity check

I wouldn't really call this a sanity check.. I have a machine with 40 logical processors, which gets it pretty close to this limit. I think this is a sane limit, but it should maybe only be applied when workerCount < 1, and should get a better comment.

::: gfx/thebes/gfxPlatform.cpp:2666
(Diff revision 1)
>  
>    return result;
>  }
>  
> +/* static */ bool
> +gfxPlatform::UsesTiling()

fwiw, I don't really like what UsesOffMainThreadCompositing is doing and what you're doing here, it seems to be that those should be non-static virtual and platform specific stuff should be done in their children. However since there's a precedence here already if you prefer this that's fine.
Attachment #8938184 - Flags: review?(bas) → review+
Comment on attachment 8938179 [details]
Lock access to PowCache in FilterNodeLighting (bug 1425056, )

https://reviewboard.mozilla.org/r/208900/#review215422
Attachment #8938179 - Flags: review?(mstange) → review+
Comment on attachment 8938180 [details]
Don't cache SourceSurface's in FilterNodeSoftware (bug 1425056, )

https://reviewboard.mozilla.org/r/208902/#review215424
Attachment #8938180 - Flags: review?(mstange) → review+
Comment on attachment 8938181 [details]
Make debug bounds checking for FilterNodeSoftware thread safe (bug 1425056, )

https://reviewboard.mozilla.org/r/208904/#review215426
Attachment #8938181 - Flags: review?(mstange) → review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d50f97d2091a
Lock access to PowCache in FilterNodeLighting (bug 1425056, r=mstange)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6882857e1bb5
Don't cache SourceSurface's in FilterNodeSoftware (bug 1425056, r=mstange)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d88e13db33b
Make debug bounds checking for FilterNodeSoftware thread safe (bug 1425056, r=mstange)
https://hg.mozilla.org/integration/mozilla-inbound/rev/af2ed1a4a02b
Report a DrawTargetTiled as a capture if it is made of captures (bug 1425056, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbfd11a64c8
Make a CaptureTiledPaintState for each tile (bug 1425056, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d96ef3b33c
Create a PaintWorker thread pool and dispatch tiles to it (bug 1425056, r=bas)
This brought some perf improvements:

== Change summary for alert #11138 (as of Wed, 10 Jan 2018 19:10:21 GMT) ==

Improvements:

  3%  rasterflood_svg windows7-32 opt e10s     16,828.14 -> 16,298.46
  2%  rasterflood_svg linux64 pgo e10s         27,986.58 -> 27,343.18
  2%  rasterflood_svg osx-10-10 opt e10s       12,895.90 -> 12,603.58
  2%  rasterflood_svg linux64 opt e10s         26,937.85 -> 26,333.24
  1%  rasterflood_svg windows10-64 opt e10s    11,807.02 -> 11,728.54

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11138
We finished testing Multi-OMTP feature on following OS-es:

- Mac OS X 10.13 : 4 Cores and pref. "layers.omtp.paint-workers = -1"
- Mac OS X 10.12 : 2 Cores and pref. "layers.omtp.paint-workers = 4"

Here is the test report: https://testrail.stage.mozaws.net/index.php?/reports/view/770.
Status: RESOLVED → VERIFIED
OS: Unspecified → Mac OS X
Depends on: 1447507
You need to log in before you can comment on or make changes to this bug.