1136766 - Skip culling if the draw region becomes more complex

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: (mstange) Part 1: Don't add draw calls when culling

Comment 2

[Benoit Girard (:BenWa)]

Attachment: (mstange) Part 2: Cull using the clip

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Effects off overculling

The red layer is split into many draw calls.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: test.html

Here's a testcase for part 1.

without part 1: 67 draw calls
With part 1: 46 draw calls

Comment 5

[Benoit Girard (:BenWa)]

Here's the try push for just part 1:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93f962e216b

Passing it back to mstange

Comment 6

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=765ee487cf20

Comment 7

[Benoit Girard (:BenWa)]

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

Comment 8

[Benoit Girard (:BenWa)]

Attachment: (mstange) Part 2: Cull using the clip (rebased)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=987d6c5e9732

Comment 9

[Benoit Girard (:BenWa)]

Attachment: 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 10

[Benoit Girard (:BenWa)]

Attachment: 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

Comment 11

[Benoit Girard (:BenWa)]

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

Comment 12

[Benoit Girard (:BenWa)]

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 13

[Benoit Girard (:BenWa)]

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

Comment 14

[Benoit Girard (:BenWa)]

Inbound is closed, currently no conflicts.

Comment 15

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/674f8f76a58bbd51297beb214e9a90f94a5cfd84
changeset:  674f8f76a58bbd51297beb214e9a90f94a5cfd84
user:       Markus Stange <[e-mail]>
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 <[e-mail]>
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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

[Benoit Girard (:BenWa)]

Attachment: assert failure

Comment 18

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

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.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

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

Comment 20

[Benoit Girard (:BenWa)]

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

Comment 21

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0ee959c4044

Comment 22

[Benoit Girard (:BenWa)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4985245dfd5

Comment 24

[Pulsebot]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Markus Stange [:mstange]]

(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.

Comment 27

[Markus Stange [: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.)

Comment 28

[Markus Stange [:mstange]]

(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.

Comment 29

[Benoit Girard (:BenWa)]

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.

Comment 30

[Firefox Bug Husbandry Bot]

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