cc: Return resources to child compositor via a Callback.

cc: Return resources to child compositor via a Callback.

Currently resources in the ResourceProvider are returned for release
back to a child compositor by explicitly calling the method
ResourceProvider::PrepareSendReturnsToChild(). This is problematic
because we want to return resources immediately if the last reference
on the resource is released when a parent compositor gives the resource
back to us.

TextureMailboxes deal with this gracefully by using a ReleaseCallback
in the ResourceProvider that is called when the mailbox is no longer in
use. We follow this example to release resources back to child
compositors by introducing a ReturnCallback in the ResourceProvider.
The ReturnCallback receives an array of resources instead of a single
resource per call, allowing us to save on post tasks and Run()
invocations.

The PrepareSendReturnsToChild is completely replaced by the new callback
mechanism. Resources are returned when they are no longer in use in the
current frame, and their exported reference count reaches zero (or lost
if the child compositor's connection is destroyed).

When the DelegatedRendererLayerImpl recieves a new frame, it will always
add all the resources into the ResourceProvider so that it can map the
resource ids correctly into its own compositor's namespace. Then it will
decide if the frame is valid or not. At that point, the DRLI must decide
to use the new frame, or continue using the old frame. It does this by
calling DeclareUsedResourcesFromChild() on the provider. Any resource
not listed in the declaration will become unused and returned to the
child as soon as possible. This changes the contract on ResourceProvider
such that every call to ReceiveFromChild() should be followed by a
DeclareUsedResourcesFromChild() call.

For now, DelegatedRendererLayer receives the callbacks about returned
resources and just inserts them in its list of resources for the
embedder to grab via TakeUnusedResourcesForChildCompositor(). A
follow-up step will be to move this callback out into the embedder
itself, similar to ReleaseCallback for mailboxes, so that the
RenderWidgetHostViewAura can promptly return resources instead of
waiting until the next commit to know resources are available.

Since resources are now returned to the main thread during commit, we
no longer have an extra pending frame of resources sitting on the impl
side, and can eliminate all the TODOs in our unit tests about this.

Covered by our many existing tests, but also added some new ones:
ResourceProviderTest.DeleteExportedResources
ResourceProviderTest.DestroyChildWithExportedResources

R=piman
BUG=263069
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224640

[piman, 2013-09-20 01:55:01]

There's the threading issue with the callback. Outside of that, there's a couple
of places where I think there's redundant code, which makes me think maybe I
missed something :(

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer.cc:68:
base::Bind(&DelegatedRendererLayer::ReceiveUnusedResources, this));
Layer is only RefCounted, not RefCountedThreadSafe. Also, it would mean we keep
a reference to this layer on the impl thread, which means we might destroy it
there, along with (potentially) its subtree. I suspect a ton of layers won't
like that.

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer_impl.cc:136:
resource_provider->DeclareUsedResourcesFromChild(child_id_, resources_);
Why do we need this? It seems like a noop. We're not changing the list of used
resources in the RP, so we won't have new unused resources. And we didn't have
unused resources before that we can return now, because we return them
immediately as soon as they're unused and back from the parent (so we would have
returned them before if we could).

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:985: // The child is gone and has abandoned
the resource.
How can this happen? If the child is gone, we removed the resource (and you test
for it on l.958), right?

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:994: resources_for_child.clear();
Not sure how "coherent" the child ids will be in practice during the returns,
and if they're not, what the cost will be to call the callback many times. Maybe
a TODO to investigate? (options are to sort the |resources| by child_id, or to
keep a per-child id map of resources to delete).

https://codereview.chromium.org/24078024/diff/4001/cc/trees/layer_tree_host_u...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest_delegated.cc:1526:
receive_resource_on_thread_);
Ouch, that is twisted. Hopefully that'll be easier after we forward the callback
to the DRL's client.

[danakj, 2013-09-20 14:54:52]

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer.cc:68:
base::Bind(&DelegatedRendererLayer::ReceiveUnusedResources, this));
On 2013/09/20 01:55:01, piman wrote:
> Layer is only RefCounted, not RefCountedThreadSafe. Also, it would mean we
keep
> a reference to this layer on the impl thread, which means we might destroy it
> there, along with (potentially) its subtree. I suspect a ton of layers won't
> like that.

Oh, right. I will use a WeakPtrFactory.

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer_impl.cc:136:
resource_provider->DeclareUsedResourcesFromChild(child_id_, resources_);
On 2013/09/20 01:55:01, piman wrote:
> Why do we need this? It seems like a noop. We're not changing the list of used
> resources in the RP, so we won't have new unused resources. And we didn't have
> unused resources before that we can return now, because we return them
> immediately as soon as they're unused and back from the parent (so we would
have
> returned them before if we could).

We did ReceiveFromChild above, which adds any resources in the frame to the
ResourceProvider. If we're not going to use the frame, we should return the
unused ones. This does not change which resources are marked as in use, but it
will return refs on resources from the invalid frame that we are not going to
use.

(This is why the rule is ReceiveFromChild always followed by
DeclareUsedResources.)

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:985: // The child is gone and has abandoned
the resource.
On 2013/09/20 01:55:01, piman wrote:
> How can this happen? If the child is gone, we removed the resource (and you
test
> for it on l.958), right?

Hm.. let me double check.. maybe this condition is wrong in the final form of
this CL.

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:994: resources_for_child.clear();
On 2013/09/20 01:55:01, piman wrote:
> Not sure how "coherent" the child ids will be in practice during the returns,
> and if they're not, what the cost will be to call the callback many times.
Maybe
> a TODO to investigate? (options are to sort the |resources| by child_id, or to
> keep a per-child id map of resources to delete).

I thought about a per-child id map but I thought it was kinda gross. Sorting is
a cool idea though!

[danakj, 2013-09-20 15:39:21]

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer.cc:68:
base::Bind(&DelegatedRendererLayer::ReceiveUnusedResources, this));
On 2013/09/20 14:54:52, danakj wrote:
> On 2013/09/20 01:55:01, piman wrote:
> > Layer is only RefCounted, not RefCountedThreadSafe. Also, it would mean we
> keep
> > a reference to this layer on the impl thread, which means we might destroy
it
> > there, along with (potentially) its subtree. I suspect a ton of layers won't
> > like that.
> 
> Oh, right. I will use a WeakPtrFactory.

Ugh thread issues. The right way to do this is to use a Holder class with a
BlockingTaskRunner and a post task. I'll see if I can avoid going all the way to
that for now.

[danakj, 2013-09-20 17:35:12]

PTAL

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer.cc:68:
base::Bind(&DelegatedRendererLayer::ReceiveUnusedResources, this));
On 2013/09/20 15:39:21, danakj wrote:
> On 2013/09/20 14:54:52, danakj wrote:
> > On 2013/09/20 01:55:01, piman wrote:
> > > Layer is only RefCounted, not RefCountedThreadSafe. Also, it would mean we
> > keep
> > > a reference to this layer on the impl thread, which means we might destroy
> it
> > > there, along with (potentially) its subtree. I suspect a ton of layers
won't
> > > like that.
> > 
> > Oh, right. I will use a WeakPtrFactory.
> 
> Ugh thread issues. The right way to do this is to use a Holder class with a
> BlockingTaskRunner and a post task. I'll see if I can avoid going all the way
to
> that for now.

Ok done. Used a WeakPtrFactory and a BlockingTaskRunner to ensure the weakptr is
destroyed on the right thread. This allows us to have the Layer destruction
timing unchanged.

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:985: // The child is gone and has abandoned
the resource.
On 2013/09/20 14:54:52, danakj wrote:
> On 2013/09/20 01:55:01, piman wrote:
> > How can this happen? If the child is gone, we removed the resource (and you
> test
> > for it on l.958), right?
> 
> Hm.. let me double check.. maybe this condition is wrong in the final form of
> this CL.

DCHECKing here pointed out that Resource::child_id was not being initialized to
0. Woops. And initializing it to 0 failed some tests, because I wasn't deleting
resources returned from the parent with a child_id = 0. Fixed both of these and
left in the DCHECK.

https://codereview.chromium.org/24078024/diff/4001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:994: resources_for_child.clear();
On 2013/09/20 14:54:52, danakj wrote:
> On 2013/09/20 01:55:01, piman wrote:
> > Not sure how "coherent" the child ids will be in practice during the
returns,
> > and if they're not, what the cost will be to call the callback many times.
> Maybe
> > a TODO to investigate? (options are to sort the |resources| by child_id, or
to
> > keep a per-child id map of resources to delete).
> 
> I thought about a per-child id map but I thought it was kinda gross. Sorting
is
> a cool idea though!

Implementing sorting required some fun with pairs, but should work.

[piman, 2013-09-20 20:14:57]

lgtm

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/24078024/diff/4001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer_impl.cc:136:
resource_provider->DeclareUsedResourcesFromChild(child_id_, resources_);
On 2013/09/20 14:54:52, danakj wrote:
> On 2013/09/20 01:55:01, piman wrote:
> > Why do we need this? It seems like a noop. We're not changing the list of
used
> > resources in the RP, so we won't have new unused resources. And we didn't
have
> > unused resources before that we can return now, because we return them
> > immediately as soon as they're unused and back from the parent (so we would
> have
> > returned them before if we could).
> 
> We did ReceiveFromChild above, which adds any resources in the frame to the
> ResourceProvider. If we're not going to use the frame, we should return the
> unused ones. This does not change which resources are marked as in use, but it
> will return refs on resources from the invalid frame that we are not going to
> use.
> 
> (This is why the rule is ReceiveFromChild always followed by
> DeclareUsedResources.)

Ok, I know what I was missing: ReceiveFromChild adds unused resources, sort of
in a limbo, that are not returned right away (so that we can "attach" to them in
DeclareUsedResourcesFromChild). So if we're not going to attach to them, we
still need DeclareUsedResourcesFromChild so that we can return them immediately.

[commit-bot: I haz the power, 2013-09-20 20:17:38]

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

[commit-bot: I haz the power, 2013-09-21 02:14:51]

Retried try job too often on win_rel for step(s) browser_tests,
content_browsertests, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-21 15:13:59]

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

[commit-bot: I haz the power, 2013-09-22 23:30:03]

Change committed as 224640