bug 1085223 added precise occlusion culling. However it has the potential for creating more complex regions: 111 111 ... 111 ... 121 -> 1.1 + .2. instead of 111 + .2. 111 111 ... 111 ... Note that layer 1 would go from 1 draw call to 4. We know that culling improves performance (bug 1132144 regression from when we backed it out for a day). But we're not sure if it improves performance because of 1) less bandwidth, 2) less draw calls or a mix of 1)+2) (this answer could be platform dependent). In this bug we should investigate not culling if we're in a situation where this culling step causes more draw calls.

The red layer is split into many draw calls.

Here's a testcase for part 1. without part 1: 67 draw calls With part 1: 46 draw calls

Here's the try push for just part 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93f962e216b Passing it back to mstange

Comment on attachment 8570645 [details] [diff] [review] (mstange) Part 1: Don't add draw calls when culling Review of attachment 8570645 [details] [diff] [review]: ----------------------------------------------------------------- First part is ready to go

Bug 1136766 - Don't allow more draw calls after culling. r=mattwoodrow

Bug 1136766 - Before compositing, clip the visible region of a layer to the layer's clip rect, and don't increase the complexity of the visible region. r=mattwoodrow

Looks like we forgot about these. Rebased part 2 and lets do another try run to make sure.

Comment on attachment 8650562 [details] MozReview Request: Bug 1136766 - Don't allow more draw calls after culling. r=mattwoodrow Bug 1136766 - Don't allow more draw calls after culling. r=mattwoodrow

Comment on attachment 8650563 [details] MozReview Request: Bug 1136766 - Before compositing, clip the visible region of a layer to the layer's clip rect, and don't increase the complexity of the visible region. r=mattwoodrow Bug 1136766 - Before compositing, clip the visible region of a layer to the layer's clip rect, and don't increase the complexity of the visible region. r=mattwoodrow

Inbound is closed, currently no conflicts.

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/674f8f76a58bbd51297beb214e9a90f94a5cfd84 changeset: 674f8f76a58bbd51297beb214e9a90f94a5cfd84 user: Markus Stange <mstange@themasta.com> date: Fri Feb 27 14:17:35 2015 -0500 description: Bug 1136766 - Don't allow more draw calls after culling. r=mattwoodrow url: https://hg.mozilla.org/integration/mozilla-inbound/rev/c574db1b372e997a8d6389ccae2e207c264d0197 changeset: c574db1b372e997a8d6389ccae2e207c264d0197 user: Markus Stange <mstange@themasta.com> date: Sat Feb 21 18:16:53 2015 -0500 description: Bug 1136766 - Before compositing, clip the visible region of a layer to the layer's clip rect, and don't increase the complexity of the visible region. r=mattwoodrow

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca399b0bba8 for OSX reftest assertions: https://treeherder.mozilla.org/logviewer.html#?job_id=13160069&repo=mozilla-inbound

we have a lot of e10s specific talos failures with this patch: http://alertmanager.allizom.org:8080/alerts.html?rev=c574db1b372e997a8d6389ccae2e207c264d0197&showAll=1&testIndex=0&platIndex=0 https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=5e2d43bdbaee&newProject=mozilla-inbound&newRevision=c574db1b372e&e10s=1 mostly in webgl terrain and tscroll asap. It would be nice to know we expect these regressions, or do some work to reduce or fix the regressions.

a real perfherder view: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=5e2d43bdbaee&newProject=mozilla-inbound&newRevision=c574db1b372e&e10s=1

These were you patch here. Are we still wanting to continue this work?

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4985245dfd55ba44e0417c42c956e03d094b0df changeset: d4985245dfd55ba44e0417c42c956e03d094b0df user: Benoit Girard <b56girard@gmail.com> date: Fri Feb 27 14:17:35 2015 -0500 description: Bug 1136766 - Don't allow more draw calls after culling. r=mattwoodrow

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/16c530a7412e

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/16c530a7412e

(In reply to Benoit Girard (:BenWa) from comment #20) > These were you patch here. Are we still wanting to continue this work? Maybe some day, it's not urgent.

https://reviewboard.mozilla.org/r/16639/#review17827 ::: gfx/layers/composite/LayerManagerComposite.cpp:257 (Diff revision 2) > - // Intersect the original region with the bounds of the culled region so > - // that we don't increase the region's complexity. > - visible.AndWith(afterCulling.GetBounds()); > + if (haveClip) { > + afterCulling.AndWith(clip); > + } These three lines shouldn't have been removed - without them, we won't actually do any culling. (See what happens with the "visible" variable: We initialize it with the current ShadowVisibleRegion, we don't mutate it, and then we set it again on the layer.)

(In reply to Pulsebot from comment #24) > Backout: > https://hg.mozilla.org/integration/mozilla-inbound/rev/16c530a7412e So the first part was backed out again because of perf regressions. The perf comparison is here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=b47b07514a11&newProject=mozilla-inbound&newRevision=d4985245dfd5 As far as I can tell, the only interesting number here is the tscrollx regression on Windows 7 and 8.

Discuss this with mstange. For now we're not really looking into this anymore. We've made some internal changes that will mitigate the problem a bit, we're not running into instances of this problem either ATM.

Removing leave-open keyword from resolved bugs, per :sylvestre.