Issue 23097005: cc: return resources via a ReturnedResource struct rather than TransferableResource

Issue 23097005: cc: return resources via a ReturnedResource struct rather than TransferableResource (Closed)

Created:
7 years, 4 months ago by piman

Modified:
7 years, 4 months ago

Reviewers:
danakj, palmer

CC:
chromium-reviews, jbauman+watch_chromium.org, yusukes+watch_chromium.org, Ian Vollick, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org, James Su, danakj+watch_chromium.org, miu+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

cc: return resources via a ReturnedResource struct rather than TransferableResource This allows us to return a count, as well as reducing the size significantly since we don't need to return the mailbox. This is built on top of https://codereview.chromium.org/23023005 BUG=263068 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218214

Patch Set 1 #

Total comments: 9

Patch Set 2 : rebase #

Patch Set 3 : post-rebase fixes #

Total comments: 12

Patch Set 4 : review nits #

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+213 lines, -189 lines)			Patch
M	cc/cc.gyp	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	cc/layers/delegated_renderer_layer.h	View	1 2 3	3 chunks	+3 lines, -3 lines	0 comments	Download
M	cc/layers/delegated_renderer_layer.cc	View	1 2 3	2 chunks	+4 lines, -5 lines	0 comments	Download
M	cc/layers/delegated_renderer_layer_impl.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	cc/layers/delegated_renderer_layer_impl.cc	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	cc/layers/texture_layer_unittest.cc	View	1 2 3	2 chunks	+4 lines, -1 line	0 comments	Download
M	cc/output/compositor_frame_ack.h	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	cc/output/delegating_renderer.cc	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	cc/resources/resource_provider.h	View	1 2 3	3 chunks	+6 lines, -5 lines	0 comments	Download
M	cc/resources/resource_provider.cc	View	1 2 3	27 chunks	+56 lines, -121 lines	0 comments	Download
M	cc/resources/resource_provider_unittest.cc	View	1 2 3	7 chunks	+19 lines, -12 lines	0 comments	Download
A	cc/resources/returned_resource.h	View	1 2 3	1 chunk	+27 lines, -0 lines	0 comments	Download
M	cc/resources/transferable_resource.h	View	1 2 3	2 chunks	+9 lines, -2 lines	0 comments	Download
M	cc/resources/transferable_resource.cc	View	1 2 3	2 chunks	+19 lines, -0 lines	0 comments	Download
M	cc/test/fake_delegated_renderer_layer_impl.cc	View	1 2 3	2 chunks	+2 lines, -1 line	0 comments	Download
M	cc/test/fake_output_surface.cc	View	1 2 3	2 chunks	+2 lines, -1 line	0 comments	Download
M	cc/trees/layer_tree_host_unittest_delegated.cc	View	1 2 3	13 chunks	+38 lines, -26 lines	0 comments	Download
M	content/browser/renderer_host/render_widget_host_impl.cc	View	1 2 3	1 chunk	+3 lines, -1 line	0 comments	Download
M	content/browser/renderer_host/render_widget_host_view_aura.cc	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
M	content/common/cc_messages.h	View	1 2 3	2 chunks	+8 lines, -0 lines	0 comments	Download
M	ui/compositor/layer.h	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	ui/compositor/layer.cc	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

piman

danakj: please review palmer: OWNERS on cc_messages.h : ReturnedResource is used in place of TransferableResource ...

7 years, 4 months ago (2013-08-14 00:18:51 UTC) #1

danakj

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (left): https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provider.cc#oldcode833 cc/resources/resource_provider.cc:833: TransferResource(context3d, *it, &resource); How do you get away without ...

7 years, 4 months ago (2013-08-15 23:16:44 UTC) #3

piman

7 years, 4 months ago (2013-08-16 05:12:44 UTC) #4

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (left):

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:833: TransferResource(context3d, *it,
&resource);
On 2013/08/15 23:16:44, danakj wrote:
> How do you get away without doing TransferResource here?

So, before the patch, TransferResource does nothing but fill in the
TransferableResource - because we already have a mailbox (when we imported),
there's no need to produce() any more.
This is no different, just inline.

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:827: list->push_back(returned);
On 2013/08/15 23:16:44, danakj wrote:
> This is going to push the resource onto the array that may already contain the
> resource. I guess this is fine, if all code knows to look for a resource more
> than once in the array?

Really, the only code that's looking at the array is ReceiveFromParent, which
handles it just fine if it happens. I'm having trouble thinking about a case
where we'd want to hand-pick an element from the list without going through all
of it.

> Or am I reading this wrong?
> 
> Should instead the DRLImpl be passing in a TransferableResourceArray to here,
> and we call ReturnResources on it there to add them to the
> ReturnedResourceArray?
> 
> Or should this be coalescing them maybe doing ReturnResources() in here or
> something?

Really, the goal of this patch is to avoid scaling problems with a list becoming
arbitrarily large because a resource is being used every frame.
So I don't want to emit each resource N times in a TransferableResourceArray
just to coalesce later.

Having duplicates can happen anyway in DRL when it gets 2 frames without a
commit. We could argue about whether we should coalesce in there too, but taking
a step back for a minute...
In almost all cases, the list is empty, so you have no duplicates. The list in
DRL is picked up after the commit. That is actually always the case in prod
currently.
Once we return resources through a callback instead, because you have a limited,
low number of frames in flight (1 currently), this bounds the number of
duplicates you could have in the most extreme edge case where multiple frames
are returned from a parent, before the commit takes back the resources. So even
from a memory standpoint there's not much to gain in trying to coalesce just in
case (since to coalesce, you have to put in a hash set anyway, which will use
more memory). The cost of coalescing seems to outweigh any savings we could have
otherwise.

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:900: DCHECK(resource->exported_count >=
it->count);
On 2013/08/15 23:16:44, danakj wrote:
> DCHECK_GE

Done.

danakj

LGTM https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (left): https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provider.cc#oldcode833 cc/resources/resource_provider.cc:833: TransferResource(context3d, *it, &resource); On 2013/08/16 05:12:44, piman wrote: ...

7 years, 4 months ago (2013-08-16 23:33:34 UTC) #5

LGTM

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (left):

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:833: TransferResource(context3d, *it,
&resource);
On 2013/08/16 05:12:44, piman wrote:
> On 2013/08/15 23:16:44, danakj wrote:
> > How do you get away without doing TransferResource here?
> 
> So, before the patch, TransferResource does nothing but fill in the
> TransferableResource - because we already have a mailbox (when we imported),
> there's no need to produce() any more.
> This is no different, just inline.

I see! I like that the returned struct doesn't have redundant data now.

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/23097005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:827: list->push_back(returned);
On 2013/08/16 05:12:44, piman wrote:
> On 2013/08/15 23:16:44, danakj wrote:
> > This is going to push the resource onto the array that may already contain
the
> > resource. I guess this is fine, if all code knows to look for a resource
more
> > than once in the array?
> 
> Really, the only code that's looking at the array is ReceiveFromParent, which
> handles it just fine if it happens. I'm having trouble thinking about a case
> where we'd want to hand-pick an element from the list without going through
all
> of it.
> 
> > Or am I reading this wrong?
> > 
> > Should instead the DRLImpl be passing in a TransferableResourceArray to
here,
> > and we call ReturnResources on it there to add them to the
> > ReturnedResourceArray?
> > 
> > Or should this be coalescing them maybe doing ReturnResources() in here or
> > something?
> 
> Really, the goal of this patch is to avoid scaling problems with a list
becoming
> arbitrarily large because a resource is being used every frame.
> So I don't want to emit each resource N times in a TransferableResourceArray
> just to coalesce later.
> 
> Having duplicates can happen anyway in DRL when it gets 2 frames without a
> commit. We could argue about whether we should coalesce in there too, but
taking
> a step back for a minute...
> In almost all cases, the list is empty, so you have no duplicates. The list in
> DRL is picked up after the commit. That is actually always the case in prod
> currently.
> Once we return resources through a callback instead, because you have a
limited,
> low number of frames in flight (1 currently), this bounds the number of
> duplicates you could have in the most extreme edge case where multiple frames
> are returned from a parent, before the commit takes back the resources. So
even
> from a memory standpoint there's not much to gain in trying to coalesce just
in
> case (since to coalesce, you have to put in a hash set anyway, which will use
> more memory). The cost of coalescing seems to outweigh any savings we could
have
> otherwise.

OK sure. I buy all this and it's nice to remember that ReceiveFromParent is the
place we care about this. Thanks for the long explanation.

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

https://codereview.chromium.org/23097005/diff/9001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:809: void
ResourceProvider::PrepareSendToChild(int child,
Maybe we could rename this to PrepareSendReturnsToChild and
ReceiveReturnsFromParent or something like that in the future. I always had to
stop and think about these method names in the past.

https://codereview.chromium.org/23097005/diff/9001/cc/resources/transferable_...
File cc/resources/transferable_resource.cc (right):

https://codereview.chromium.org/23097005/diff/9001/cc/resources/transferable_...
cc/resources/transferable_resource.cc:20: ReturnedResource
TransferableResource::Return() const {
nit: ToReturnedResource?

https://codereview.chromium.org/23097005/diff/9001/cc/resources/transferable_...
File cc/resources/transferable_resource.h (right):

https://codereview.chromium.org/23097005/diff/9001/cc/resources/transferable_...
cc/resources/transferable_resource.h:35: struct CC_EXPORT ReturnedResource {
Can you move this to its own file please?

https://codereview.chromium.org/23097005/diff/9001/cc/resources/transferable_...
cc/resources/transferable_resource.h:45: CC_EXPORT void ReturnResources(const
TransferableResourceArray& input,
I'm kinda meh about a free function like this, as it's hard to tell where the
function comes from when you look at the call sites.

WDYT of making ReturnedResourceArray a subclass of std::vector<RR>, and making
this a (static?) method of the class?

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

https://codereview.chromium.org/23097005/diff/9001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest_delegated.cc:51: result = false;
why not just return false here? If the id's don't match does the rest matter?

https://codereview.chromium.org/23097005/diff/9001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest_delegated.cc:54: result = false;
same. if you agree returning immediately is cleaner, you can probably drop the
temp result var entirely.

piman

https://codereview.chromium.org/23097005/diff/9001/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/23097005/diff/9001/cc/resources/resource_provider.cc#newcode809 cc/resources/resource_provider.cc:809: void ResourceProvider::PrepareSendToChild(int child, On 2013/08/16 23:33:35, danakj wrote: > ...

7 years, 4 months ago (2013-08-17 01:54:30 UTC) #6