642246 - Don't build ThebesLayers for elements with no displayports

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Stover (:stechz)]

The compositing process needs to know about all scrollable elements, but it is expensive to build ThebesLayers for every one of them. We can save a lot of memory by not building a ThebesLayer for elements without displayports. This means that at first a scroll frame will not be able to scroll asynchronously but as soon as the displayport is updated, it will work fine.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Don't build ThebesLayers for elements with no displayports

The first step is just to build an empty nsDisplayScrollLayer with no items
inside it. Since we already have a FORCE LAYER flag, it was easy to let the
container layer be built even when nothing was inside of it. This has the
consequence of making ContainerLayers able to have no children, so I had to
add an AsContainerLayer method. Everything else worked magically for Fennec!

Important thing to consider: is there any other code that makes assumptions
about ContainerLayers always having children?

Comment 2

[Benjamin Stover (:stechz)]

Comment on attachment 519748 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

Hm, actually before I ask for reviews. Roc: do you think this approach is a good idea?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #1)
> Important thing to consider: is there any other code that makes assumptions
> about ContainerLayers always having children?

I hope not. If there is, we should fix it.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This approach looks OK.

+      if (itemVisibleRect.IsEmpty() && layerState != LAYER_ACTIVE_FORCE) {

Do you really want to create a layer even if the scrollframe is completely covered by other opaque content?

Comment 5

[Benjamin Stover (:stechz)]

(In reply to comment #4)
> This approach looks OK.
> 
> +      if (itemVisibleRect.IsEmpty() && layerState != LAYER_ACTIVE_FORCE) {
> 
> Do you really want to create a layer even if the scrollframe is completely
> covered by other opaque content?

Oh, probably not. I added this because the itemVisibleRect is of course empty with no items inside it.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I see. I think you should arrange for the visible rect of your item to be correctly computed to be the whole scrollable area instead.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Don't build ThebesLayers for elements with no displayports

Comment 8

[Benjamin Stover (:stechz)]

(In reply to comment #6)
> I see. I think you should arrange for the visible rect of your item to be
> correctly computed to be the whole scrollable area instead.

Hm, I'm not sure that's what we want since those display items are going to also be in another layer. The point of this layer is to only have the metadata for the scroll frame.

I've made a new layer type called LAYER_ACTIVE_EMPTY that is explicitly for metadata, so that we have the expectation that its visible rect should always be empty. Now it's easy to tell the difference between an occluded layer and an intentionally empty layer. What do you think?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

I like it!

Do you need to do something to ensure that when a displayport is set on an element, we invalidate it so that the nsDisplayScrollLayer is created?

Comment 10

[Benjamin Stover (:stechz)]

(In reply to comment #9)
> Comment on attachment 524452 [details] [diff] [review]
> Don't build ThebesLayers for elements with no displayports
> 
> I like it!
> 
> Do you need to do something to ensure that when a displayport is set on an
> element, we invalidate it so that the nsDisplayScrollLayer is created?

We already do that, so we're set there. :) So, time to get a review on this. Maybe Timothy is the best?

Comment 11

[Benjamin Stover (:stechz)]

Comment on attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

Actually, roc, you okay with formally reviewing this since you sort of know the patch?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

+    if (!usingDisplayport) {

Can you flip this around so that it's "if (usingDisplayport)"?

Comment 13

[Benjamin Stover (:stechz)]

Try server: http://tbpl.mozilla.org/?tree=MozillaTry&rev=e6347821d785

Comment 14

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mozilla-central/rev/1edfae12015b

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out due to massive mobile unittest orange following cedar merge (as the most suspicious candidate):
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=fe3f7889918b

Comment 16

[Matt Brubeck (:mbrubeck)]

Re-landed on 2011-04-11: http://hg.mozilla.org/mozilla-central/rev/ede9a23ec516