Closed Bug 1136766 Opened 7 years ago Closed 5 years ago

Skip culling if the draw region becomes more complex

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

(Whiteboard: [talos_regression][e10s][gfx-noted])

Attachments

(7 files, 1 obsolete file)

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.
Whiteboard: [gfx-noted]
Blocks: 1088154
Attached image Effects off overculling
The red layer is split into many draw calls.
Attached file test.html
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
Attachment #8570645 - Flags: review?(matt.woodrow)
Attachment #8570645 - Flags: review?(matt.woodrow) → review+
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.
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
Keywords: checkin-needed
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
Keywords: checkin-needed
Attached file assert failure
Flags: needinfo?(bgirard)
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.
Whiteboard: [gfx-noted] → [talos_regression][e10s][gfx-noted]
These were you patch here. Are we still wanting to continue this work?
Flags: needinfo?(mstange)
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
Keywords: leave-open
(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.
Flags: needinfo?(mstange)
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.