Bug 90702 – [chromium] Avoid allocating render pass textures that have no content

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Avoid allocating render pass textures that have no content

[Dana Jansens]

Created attachment 151119 [details]
Patch

[Dana Jansens]

Created attachment 151120 [details]
Patch

[Adrienne Walker]

Comment on attachment 151120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review

This feels a little over-engineered to me.  There's only two types of culling here, and they both could iterate in the same way, starting from the root, yeah?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487
>              // We are changing the vector in the middle of reverse iteration.
>              // We are guaranteed that any data from iterator to the end will not change.
>              // Capture the iterator position from the end, and restore it after the change.
> -            int positionFromEnd = passes.size() - passIndex;
> -            removeRenderPassesRecursive(passes, passIndex, renderPassQuad->renderPass(), skippedPasses);
> -            passIndex = passes.size() - positionFromEnd;
> -            ASSERT(passIndex >= 0);
> +            int positionFromEnd = frame.renderPasses.size() - it;
> +            removeRenderPassesRecursive(frame.renderPasses, it, renderPassQuad->renderPass(), frame.skippedPasses);
> +            it = frame.renderPasses.size() - positionFromEnd;
> +            ASSERT(it >= 0);

How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0.

[Dana Jansens]

(In reply to comment #3)
> (From update of attachment 151120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review
> 
> This feels a little over-engineered to me.  There's only two types of culling here, and they both could iterate in the same way, starting from the root, yeah?

I think no :/ In the case of culling empty lists, we want to go in dependency order, because making one empty can make its parent empty. In the cached texture case, dropping the root should drop its subtree. The relationship in the empty case is reversed. We drop the parent based on the decision to drop its child (does dropping its child make the parent empty?).

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487
> >              // We are changing the vector in the middle of reverse iteration.
> >              // We are guaranteed that any data from iterator to the end will not change.
> >              // Capture the iterator position from the end, and restore it after the change.
> > -            int positionFromEnd = passes.size() - passIndex;
> > -            removeRenderPassesRecursive(passes, passIndex, renderPassQuad->renderPass(), skippedPasses);
> > -            passIndex = passes.size() - positionFromEnd;
> > -            ASSERT(passIndex >= 0);
> > +            int positionFromEnd = frame.renderPasses.size() - it;
> > +            removeRenderPassesRecursive(frame.renderPasses, it, renderPassQuad->renderPass(), frame.skippedPasses);
> > +            it = frame.renderPasses.size() - positionFromEnd;
> > +            ASSERT(it >= 0);
> 
> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0.

Ya, hm. Lemme look at that again.

[Dana Jansens]

Comment on attachment 151120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487
>>> +            ASSERT(it >= 0);
>> 
>> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0.
> 
> Ya, hm. Lemme look at that again.

The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well.

[Dana Jansens]

(In reply to comment #5)
> (From update of attachment 151120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487
> >>> +            ASSERT(it >= 0);
> >> 
> >> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0.
> > 
> > Ya, hm. Lemme look at that again.
> 
> The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well.

More specifically, to remove renderPass[0], you have to be at position >= 1.

At 1, you get:
size = N
positionFromEnd = size - 1 = N - 1
size = size - 1 = N - 1
it = size - positionFromEnd = N-1 - N-1 = 0

[Adrienne Walker]

Comment on attachment 151120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review

In that case, looks good, except for needing some more comments:

>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487
>>>>> +            ASSERT(it >= 0);
>>>> 
>>>> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0.
>>> 
>>> Ya, hm. Lemme look at that again.
>> 
>> The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well.
> 
> More specifically, to remove renderPass[0], you have to be at position >= 1.
> 
> At 1, you get:
> size = N
> positionFromEnd = size - 1 = N - 1
> size = size - 1 = N - 1
> it = size - positionFromEnd = N-1 - N-1 = 0

Ok, I'll buy that.  Sorry for the misunderstanding.  :)

I think this comment needs to be updated then, since it's not about changing something in the middle of reverse iteration, it's about removeRenderPassesRecursive only removing render passes at a lower index.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:199
> +        // Iterates in draw order.

Can you explain the 'why' and not just the 'what' for each of these culling functor classes?

[Dana Jansens]

Comment on attachment 151120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review

>>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487
>>>>>> +            ASSERT(it >= 0);
>>>>> 
>>>>> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0.
>>>> 
>>>> Ya, hm. Lemme look at that again.
>>> 
>>> The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well.
>> 
>> More specifically, to remove renderPass[0], you have to be at position >= 1.
>> 
>> At 1, you get:
>> size = N
>> positionFromEnd = size - 1 = N - 1
>> size = size - 1 = N - 1
>> it = size - positionFromEnd = N-1 - N-1 = 0
> 
> Ok, I'll buy that.  Sorry for the misunderstanding.  :)
> 
> I think this comment needs to be updated then, since it's not about changing something in the middle of reverse iteration, it's about removeRenderPassesRecursive only removing render passes at a lower index.

Yep done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:199
>> +        // Iterates in draw order.
> 
> Can you explain the 'why' and not just the 'what' for each of these culling functor classes?

yep!

[Dana Jansens]

Created attachment 151337 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151337 [details]
Patch

Attachment 151337 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13170288

[Dana Jansens]

Created attachment 151345 [details]
Patch

Hm, linked okay locally :/  Added defines for the each version of the template function to the cpp to try linking again on the bots.

[Adrienne Walker]

Comment on attachment 151345 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 151345 [details]
Patch

Clearing flags on attachment: 151345

Committed r122252: <http://trac.webkit.org/changeset/122252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.