cc: Unused resources should be given back to the child compositor

cc: Unused resources should be given back to the child compositor

If a frame passes ownership of a resource, but no quads in the frame use
it, the resource should be passed back to the child, not held forever.

If a new frame is given to the delegated renderer layer before the
taking unused resources from it, they are re-added to the new frame
in case it uses any of them. The resources not used by the new frame
will be returned after the next commit.

Tested by existing delegated renderer tests, and also by:
LayerTreeHostDelegatedTestUnnamedResource.runSingleThread
LayerTreeHostDelegatedTestUnnamedResource.runMultiThread
LayerTreeHostDelegatedTestDontLeakResource.runSingleThread
LayerTreeHostDelegatedTestDontLeakResource.runMultiThread
LayerTreeHostDelegatedTestResourceSentToParent.runSingleThread
LayerTreeHostDelegatedTestResourceSentToParent.runMultiThread
LayerTreeHostDelegatedTestCommitWithoutTake.runSingleThread
LayerTreeHostDelegatedTestCommitWithoutTake.runMultiThread

R=piman
BUG=123444
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186246

[piman, 2013-03-04 17:20:45]

https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
File cc/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
cc/delegated_renderer_layer_impl.cc:119: unused_resources.push_back(it->second);
How do these get fed back into the next frame if it starts needing them again?

[danakj, 2013-03-04 17:39:27]

https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
File cc/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
cc/delegated_renderer_layer_impl.cc:119: unused_resources.push_back(it->second);
On 2013/03/04 17:20:45, piman wrote:
> How do these get fed back into the next frame if it starts needing them again?

If the next frame needs them, I would expect it to include them in the
TransferableResources for that frame.

[piman, 2013-03-04 19:05:31]

On 2013/03/04 17:39:27, danakj wrote:
>
https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
> File cc/delegated_renderer_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
> cc/delegated_renderer_layer_impl.cc:119:
unused_resources.push_back(it->second);
> On 2013/03/04 17:20:45, piman wrote:
> > How do these get fed back into the next frame if it starts needing them
again?
> 
> If the next frame needs them, I would expect it to include them in the
> TransferableResources for that frame.

This makes 2 assumptions:
1- the commit will return exactly the resources diff between the 2 frames
2- the embedder will call TakeUnusedResourcesForChildCompositor (and send an
ack) exactly once after each commit which set a new frame

1 is true now but we can't rely on it forever, because of impl-side painting,
and/or when more than 2 levels of compositing (<webview>, etc.)
2 isn't true with the resize / commit lock flow: when the UI resizes the web
contents (asking the renderer to produce a new frame at that size), we lock
commits until we receive a frame of the right size (to avoid gutters). If in the
mean time the renderer sent a frame that's the old size (before it handled the
resize message), we can't unblock the commit, but we still need to send an ack,
otherwise we'll never receive the frame of the right size.

[danakj, 2013-03-04 19:33:50]

On 2013/03/04 19:05:31, piman wrote:
> On 2013/03/04 17:39:27, danakj wrote:
> >
>
https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
> > File cc/delegated_renderer_layer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_laye...
> > cc/delegated_renderer_layer_impl.cc:119:
> unused_resources.push_back(it->second);
> > On 2013/03/04 17:20:45, piman wrote:
> > > How do these get fed back into the next frame if it starts needing them
> again?
> > 
> > If the next frame needs them, I would expect it to include them in the
> > TransferableResources for that frame.
> 
> This makes 2 assumptions:
> 1- the commit will return exactly the resources diff between the 2 frames
> 2- the embedder will call TakeUnusedResourcesForChildCompositor (and send an
> ack) exactly once after each commit which set a new frame
> 
> 1 is true now but we can't rely on it forever, because of impl-side painting,
> and/or when more than 2 levels of compositing (<webview>, etc.)

What if I return the set of resources that are a) not in the current frame's
quads, and b) not marked as inUseByConsumer in resource provider?

> 2 isn't true with the resize / commit lock flow: when the UI resizes the web
> contents (asking the renderer to produce a new frame at that size), we lock
> commits until we receive a frame of the right size (to avoid gutters). If in
the
> mean time the renderer sent a frame that's the old size (before it handled the
> resize message), we can't unblock the commit, but we still need to send an
ack,
> otherwise we'll never receive the frame of the right size.

If the commit is blocked, does that mean pushPropertiesTo() is not called until
we get a frame of the right size?

If that is true, I think we're okay? My reasoning:
- The main thread layer will accumulate the transferred resources sent for
frames of the wrong size and eventually the frame of the correct size.
- TakeUnusedResourcesForChildCompositor() won't return anything because we
didn't pushProperties yet so the resources_for_ack hasn't been collected at all.
- Once we get a frame of the right size, we'll pushPropertiesTo(). The impl
layer will hold onto resources used by the last, correct, frame. Resources from
previous, incorrect, frames that aren't used will be released back to the main
thread.
- The next ack will contain those resources.

[piman, 2013-03-04 20:40:57]

On Mon, Mar 4, 2013 at 11:33 AM, <danakj@chromium.org> wrote:

> On 2013/03/04 19:05:31, piman wrote:
>
>> On 2013/03/04 17:39:27, danakj wrote:
>> >
>>
>
> https://codereview.chromium.**org/12385085/diff/2001/cc/**
>
delegated_renderer_layer_impl.**cc<https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_layer_impl.cc>
>
>> > File cc/delegated_renderer_layer_**impl.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/12385085/diff/2001/cc/**
>
delegated_renderer_layer_impl.**cc#newcode119<https://codereview.chromium.org/12385085/diff/2001/cc/delegated_renderer_layer_impl.cc#newcode119>
>
>> > cc/delegated_renderer_layer_**impl.cc:119:
>> unused_resources.push_back(it-**>second);
>> > On 2013/03/04 17:20:45, piman wrote:
>> > > How do these get fed back into the next frame if it starts needing
>> them
>> again?
>> >
>> > If the next frame needs them, I would expect it to include them in the
>> > TransferableResources for that frame.
>>
>
>  This makes 2 assumptions:
>> 1- the commit will return exactly the resources diff between the 2 frames
>> 2- the embedder will call TakeUnusedResourcesForChildCom**positor (and
>> send an
>> ack) exactly once after each commit which set a new frame
>>
>
>  1 is true now but we can't rely on it forever, because of impl-side
>> painting,
>> and/or when more than 2 levels of compositing (<webview>, etc.)
>>
>
> What if I return the set of resources that are a) not in the current
> frame's
> quads, and b) not marked as inUseByConsumer in resource provider?


That'll work for <webview>, though I don't think that'll have an effect for
impl-side. In either case, that will delay the return of the resources
until a later time (e.g. next commit).
Note: I'm not saying we should fix this now, I just want to make sure we're
not going into a direction that's going to make it harder.


>  2 isn't true with the resize / commit lock flow: when the UI resizes the
>> web
>> contents (asking the renderer to produce a new frame at that size), we
>> lock
>> commits until we receive a frame of the right size (to avoid gutters). If
>> in
>>
> the
>
>> mean time the renderer sent a frame that's the old size (before it
>> handled the
>> resize message), we can't unblock the commit, but we still need to send an
>>
> ack,
>
>> otherwise we'll never receive the frame of the right size.
>>
>
> If the commit is blocked, does that mean pushPropertiesTo() is not called
> until
> we get a frame of the right size?
>
> If that is true, I think we're okay? My reasoning:
> - The main thread layer will accumulate the transferred resources sent for
> frames of the wrong size and eventually the frame of the correct size.
> - TakeUnusedResourcesForChildCom**positor() won't return anything because
> we
> didn't pushProperties yet so the resources_for_ack hasn't been collected
> at all.
> - Once we get a frame of the right size, we'll pushPropertiesTo(). The impl
> layer will hold onto resources used by the last, correct, frame. Resources
> from
> previous, incorrect, frames that aren't used will be released back to the
> main
> thread.
> - The next ack will contain those resources.
>

I should mention that the commit lock has a timeout after which we'll
gutter. That means we can get a commit that won't generate an ack, but
cause unused resources to go into the recycling queue, that the renderer
won't know about.

The delayed recycle (for <webview>) will be similar. Frame A adds and uses
resource 1. Commit, sends ack to child, draw/send to parent. Frame B
doesn't use resource 1 anymore, commit, can't return resource 1 yet, sends
ack to child, draw/send to parent, gets ack from parent (returning resource
1). Another commit (unrelated to the renderer) puts resource 1 into the
recycling queue. Frame C uses resource 1 without adding it (since renderer
didn't receive it back), commit, boom.

Maybe we can individually solve every single one of these problems by
adding more complicated logic in the embedder (because it needs to know
about the internal state of cc/the layer without being able to directly
access it), but I'm really worried that it'd be very hard to reason about,
and test to defend against (I don't feel capable of thinking about every
possible edge case).

I would rather have a consistent state with clear semantics - from the
embedder pov, a resource is either owned by the DRL or not. It is given by
SetFrameData, it is taken out by TakeUnusedResourcesForChildCompositor, in
whatever order those are called, especially wrt
commit/activation/send-to-parent flow. That way, whatever logic the
embedder uses, it only has to guarantee one thing, which is that the frame
doesn't use a resource that isn't owned by the DRL (without adding it with
the frame).



>
https://codereview.chromium.**org/12385085/<https://codereview.chromium.org/1...
>

[danakj, 2013-03-05 19:11:26]

PTAL

[piman, 2013-03-05 19:30:51]

lgtm

https://codereview.chromium.org/12385085/diff/16004/cc/delegated_renderer_lay...
File cc/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/12385085/diff/16004/cc/delegated_renderer_lay...
cc/delegated_renderer_layer.cc:84:
unused_resources_for_child_compositor_.clear();
Note: we have some area for optimization here - maybe tackle later on - for
cases where we will SetFrameData and then TakeUnusedResourcesForChildCompositor
before the commit (e.g. resize case), which might cause resources to accumulate
for a long time. Worth a comment / todo.

[danakj, 2013-03-05 19:39:14]

Thanks!

https://codereview.chromium.org/12385085/diff/16004/cc/delegated_renderer_lay...
File cc/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/12385085/diff/16004/cc/delegated_renderer_lay...
cc/delegated_renderer_layer.cc:84:
unused_resources_for_child_compositor_.clear();
On 2013/03/05 19:30:51, piman wrote:
> Note: we have some area for optimization here - maybe tackle later on - for
> cases where we will SetFrameData and then
TakeUnusedResourcesForChildCompositor
> before the commit (e.g. resize case), which might cause resources to
accumulate
> for a long time. Worth a comment / todo.

sgtm

[commit-bot: I haz the power, 2013-03-05 19:41:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12385085/11006

[commit-bot: I haz the power, 2013-03-05 19:48:02]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-03-05 20:20:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12385085/11006

[commit-bot: I haz the power, 2013-03-05 20:27:35]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-05 20:32:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12385085/11006

[commit-bot: I haz the power, 2013-03-05 20:39:01]

Change committed as 186246