cc: Keep track of busy resources in ResourcePool

cc: Keep track of busy resources in ResourcePool

Instead of assuming every released resource could be potentially
reusable, manage a list of busy resources and a list of immediately
reusable resources. A busy resource is one which can not be locked for
write.
    
Recheck busy resources before the tile manager is to schedule new tasks,
in AssignGpuMemoryToTiles().
    
If this operation becomes too expensive, the CheckBusyResources()
function should only be called if some resource(s) are returned from the
parent compositor or when ResourcePool releases some resource.

BUG=

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

[jadahl, 2013-10-25 10:04:45]

Hi,

This is an optimization that makes TileManager::ManageTiles take less time when
using the delegating renderer, because it will spend less time iterating through
exported resources when acquiring resources when creating raster tasks.

Some measurement using trace events in ContentShell.apk shows that ManageTiles
average time can goes from 4-5 ms to 1.5-3 ms. I also could observe very high
amount of time spent in a single ManageTiles call (20 ms, 40 ms) after some
panning back and forth, but I did not see that after this patch.

[reveman, 2013-10-25 14:16:59]

On 2013/10/25 10:04:45, jadahl wrote:
> Hi,
> 
> This is an optimization that makes TileManager::ManageTiles take less time
when
> using the delegating renderer, because it will spend less time iterating
through
> exported resources when acquiring resources when creating raster tasks.
> 
> Some measurement using trace events in ContentShell.apk shows that ManageTiles
> average time can goes from 4-5 ms to 1.5-3 ms. I also could observe very high
> amount of time spent in a single ManageTiles call (20 ms, 40 ms) after some
> panning back and forth, but I did not see that after this patch.

I assume that the "ResourceProvider::CanLockForWrite(id)" check is what is
preventing us from reusing these resources while exported, correct?

If so, can we generalize this patch a bit and think of these resources as
locked_resources or locked_for_read_resources in the resource pool and
essentially turn the CanLockForWrite() check in AcquireResource into a DCHECK?

Also, is the RP client interface really necessary? Could we instead add a
CheckLockedResources() function to the ResourcePool class that we call once in
TileManager::AssignGpuMemoryToTiles to move resources from locked_resources_ to
unused_resource_? That should be enough to make sure unused_resources_ is up to
date when it matters.

[jadahl, 2013-10-25 14:52:10]

On 2013/10/25 14:16:59, David Reveman wrote:
> On 2013/10/25 10:04:45, jadahl wrote:
> > Hi,
> > 
> > This is an optimization that makes TileManager::ManageTiles take less time
> when
> > using the delegating renderer, because it will spend less time iterating
> through
> > exported resources when acquiring resources when creating raster tasks.
> > 
> > Some measurement using trace events in ContentShell.apk shows that
ManageTiles
> > average time can goes from 4-5 ms to 1.5-3 ms. I also could observe very
high
> > amount of time spent in a single ManageTiles call (20 ms, 40 ms) after some
> > panning back and forth, but I did not see that after this patch.
> 
> I assume that the "ResourceProvider::CanLockForWrite(id)" check is what is
> preventing us from reusing these resources while exported, correct?

Yes, that is correct.

> 
> If so, can we generalize this patch a bit and think of these resources as
> locked_resources or locked_for_read_resources in the resource pool and
> essentially turn the CanLockForWrite() check in AcquireResource into a DCHECK?

This sounds like it should work as well, and it'd make the client unnecessary as
a bonus.

> 
> Also, is the RP client interface really necessary? Could we instead add a
> CheckLockedResources() function to the ResourcePool class that we call once in
> TileManager::AssignGpuMemoryToTiles to move resources from locked_resources_
to
> unused_resource_? That should be enough to make sure unused_resources_ is up
to
> date when it matters.

Limiting it to exported or not was because it was by far the most common reason
for CanLockForWrite() to return false. It was easy to add triggers for when that
state changed, that could be communicated via the client. One reason it is a
benefit is that Returned/Exported was called much less frequent than Acquire so
putting the list iteration there was an improvement, but AssignGpuMemoryToTiles
seems to not be much more frequent so I'll give it a try.

[piman, 2013-10-25 21:51:10]

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
cc/resources/resource_pool.cc:43: if (resource_provider_.get())
Can this be NULL? Below we SetResourceUsageLimits which will destroy the
resources, which will deref resource_provider_. So resource_provider_ has to be
still there. If it's not, there's bigger problems (e.g. we would leak
resources).
So I don't think resource_provider_ needs to be a weakptr at all.

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
cc/resources/resource_pool.cc:89: if
(resource_provider_->IsExported(resource->id()))
In PrioritizedResourceManager, we use InUseByConsumer(id) && !IsLost(id) for the
same purpose. Can we use the same pattern in both?

Actually, InUseByConsumer seems like the right choice anyway, because if it's
lost, then it's effectively still exported (i.e. destroying the resource won't
destroy anything until the parent destroys it too, which it won't because it's
lost), so maybe we can just use that?

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
cc/resources/resource_pool.cc:146: void
ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
I'm not a big fan of this. We'll pay that cost for each resource on each frame,
whereas discriminating exported vs non is only needed when we need to actually
discard resources, which is less frequent.

We could do that triage in ReduceResourceUsage when we know we're above the
limit, that way, no need to introduce a client to RP.

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

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:51: : public
base::SupportsWeakPtr<ResourceProvider> {
we want to remove the use of SupportsWeakPtr in the codebase, because it's
fundamentally unsafe. Instead, add a WeakPtrFactory as the last field. However,
see my comments in resource_pool.cc, I don't think you need weak ptrs (and I'd
rather we didn't).

[jadahl, 2013-10-28 09:59:29]

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
cc/resources/resource_pool.cc:43: if (resource_provider_.get())
On 2013/10/25 21:51:11, piman wrote:
> Can this be NULL? Below we SetResourceUsageLimits which will destroy the
> resources, which will deref resource_provider_. So resource_provider_ has to
be
> still there. If it's not, there's bigger problems (e.g. we would leak
> resources).
> So I don't think resource_provider_ needs to be a weakptr at all.

I couldn't see any clearly structured order of which ResourcePool and
ResourceProvider would be destructed. ResourceProvider is owned by
LayerTreeHostImpl while ResourcePool is owned by TileManager which itself is
owned by LayerTreeHostImpl. As there was no explicit requirement (as far as I
could see in the code) that ResourceProvider would be destructed after
ResourcePool I added a weak pointer in order not to rely on this implicit
behavior.

Anyhow, as regarding reveman's comment from friday, we can probably drop the
ResourceProviderClient, including the weak pointer, and just update the list
from ManageTiles before scheduling tasks.

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
cc/resources/resource_pool.cc:89: if
(resource_provider_->IsExported(resource->id()))
On 2013/10/25 21:51:11, piman wrote:
> In PrioritizedResourceManager, we use InUseByConsumer(id) && !IsLost(id) for
the
> same purpose. Can we use the same pattern in both?
> 
> Actually, InUseByConsumer seems like the right choice anyway, because if it's
> lost, then it's effectively still exported (i.e. destroying the resource won't
> destroy anything until the parent destroys it too, which it won't because it's
> lost), so maybe we can just use that?

As mentioned by reveman we can probably use CanLockForWrite here, sorting
resources in "unused" and "unlockable" resources. This will itself check if the
resource is exported or not, and we'll cover more cases where AcquireResource
would iterate through unlockable resources.

How does that sound to you?

https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
cc/resources/resource_pool.cc:146: void
ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
On 2013/10/25 21:51:11, piman wrote:
> I'm not a big fan of this. We'll pay that cost for each resource on each
frame,
> whereas discriminating exported vs non is only needed when we need to actually
> discard resources, which is less frequent.
> 
> We could do that triage in ReduceResourceUsage when we know we're above the
> limit, that way, no need to introduce a client to RP.

As mentioned before, we could probably rebuild the lists in
TileManager::AssignGpuMemoryToTiles() instead, which would drop the need for a
client.

[jadahl, 2013-10-28 14:33:48]

On 2013/10/28 09:59:29, jadahl wrote:
> https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
> File cc/resources/resource_pool.cc (right):
> 
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> cc/resources/resource_pool.cc:43: if (resource_provider_.get())
> On 2013/10/25 21:51:11, piman wrote:
> > Can this be NULL? Below we SetResourceUsageLimits which will destroy the
> > resources, which will deref resource_provider_. So resource_provider_ has to
> be
> > still there. If it's not, there's bigger problems (e.g. we would leak
> > resources).
> > So I don't think resource_provider_ needs to be a weakptr at all.
> 
> I couldn't see any clearly structured order of which ResourcePool and
> ResourceProvider would be destructed. ResourceProvider is owned by
> LayerTreeHostImpl while ResourcePool is owned by TileManager which itself is
> owned by LayerTreeHostImpl. As there was no explicit requirement (as far as I
> could see in the code) that ResourceProvider would be destructed after
> ResourcePool I added a weak pointer in order not to rely on this implicit
> behavior.
> 
> Anyhow, as regarding reveman's comment from friday, we can probably drop the
> ResourceProviderClient, including the weak pointer, and just update the list
> from ManageTiles before scheduling tasks.
> 
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> cc/resources/resource_pool.cc:89: if
> (resource_provider_->IsExported(resource->id()))
> On 2013/10/25 21:51:11, piman wrote:
> > In PrioritizedResourceManager, we use InUseByConsumer(id) && !IsLost(id) for
> the
> > same purpose. Can we use the same pattern in both?
> > 
> > Actually, InUseByConsumer seems like the right choice anyway, because if
it's
> > lost, then it's effectively still exported (i.e. destroying the resource
won't
> > destroy anything until the parent destroys it too, which it won't because
it's
> > lost), so maybe we can just use that?
> 
> As mentioned by reveman we can probably use CanLockForWrite here, sorting
> resources in "unused" and "unlockable" resources. This will itself check if
the
> resource is exported or not, and we'll cover more cases where AcquireResource
> would iterate through unlockable resources.
> 
> How does that sound to you?
> 
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> cc/resources/resource_pool.cc:146: void
> ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
> On 2013/10/25 21:51:11, piman wrote:
> > I'm not a big fan of this. We'll pay that cost for each resource on each
> frame,
> > whereas discriminating exported vs non is only needed when we need to
actually
> > discard resources, which is less frequent.
> > 
> > We could do that triage in ReduceResourceUsage when we know we're above the
> > limit, that way, no need to introduce a client to RP.
> 
> As mentioned before, we could probably rebuild the lists in
> TileManager::AssignGpuMemoryToTiles() instead, which would drop the need for a
> client.

So I gave the idea reveman's suggestion a try but it didn't turn out very nice.
This is because if we'd rebuild the unused and unlockable/exported list every
AssignGpuMemoryToTiles we'd for the most part just build an identical list, as
we would usually release an already exported resource, and , wasting lots of
time accomplishing nothing.

I think the best we can do right now is to as piman suggested and only care
about returned resources when we try to reduce memory and add a resource to
either a unused list or an exported list when the ResourcePool releases a
resource. In ReduceResourceUsage, if unused_resources_ is empty, we'd check if
exported resources are still exported and if not release them as well. Correct
me if I'm wrong, but this way we shouldn't use CanLockForWrite() but
InUseByConsumer().

However, this way we won't immediately get back "unused" resources (i.e.
released but exported) immediately after they are returned from the parent
compositor.

[reveman, 2013-10-28 16:18:08]

On 2013/10/28 14:33:48, jadahl wrote:
> On 2013/10/28 09:59:29, jadahl wrote:
> >
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
> > File cc/resources/resource_pool.cc (right):
> > 
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > cc/resources/resource_pool.cc:43: if (resource_provider_.get())
> > On 2013/10/25 21:51:11, piman wrote:
> > > Can this be NULL? Below we SetResourceUsageLimits which will destroy the
> > > resources, which will deref resource_provider_. So resource_provider_ has
to
> > be
> > > still there. If it's not, there's bigger problems (e.g. we would leak
> > > resources).
> > > So I don't think resource_provider_ needs to be a weakptr at all.
> > 
> > I couldn't see any clearly structured order of which ResourcePool and
> > ResourceProvider would be destructed. ResourceProvider is owned by
> > LayerTreeHostImpl while ResourcePool is owned by TileManager which itself is
> > owned by LayerTreeHostImpl. As there was no explicit requirement (as far as
I
> > could see in the code) that ResourceProvider would be destructed after
> > ResourcePool I added a weak pointer in order not to rely on this implicit
> > behavior.
> > 
> > Anyhow, as regarding reveman's comment from friday, we can probably drop the
> > ResourceProviderClient, including the weak pointer, and just update the list
> > from ManageTiles before scheduling tasks.
> > 
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > cc/resources/resource_pool.cc:89: if
> > (resource_provider_->IsExported(resource->id()))
> > On 2013/10/25 21:51:11, piman wrote:
> > > In PrioritizedResourceManager, we use InUseByConsumer(id) && !IsLost(id)
for
> > the
> > > same purpose. Can we use the same pattern in both?
> > > 
> > > Actually, InUseByConsumer seems like the right choice anyway, because if
> it's
> > > lost, then it's effectively still exported (i.e. destroying the resource
> won't
> > > destroy anything until the parent destroys it too, which it won't because
> it's
> > > lost), so maybe we can just use that?
> > 
> > As mentioned by reveman we can probably use CanLockForWrite here, sorting
> > resources in "unused" and "unlockable" resources. This will itself check if
> the
> > resource is exported or not, and we'll cover more cases where
AcquireResource
> > would iterate through unlockable resources.
> > 
> > How does that sound to you?
> > 
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > cc/resources/resource_pool.cc:146: void
> > ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
> > On 2013/10/25 21:51:11, piman wrote:
> > > I'm not a big fan of this. We'll pay that cost for each resource on each
> > frame,
> > > whereas discriminating exported vs non is only needed when we need to
> actually
> > > discard resources, which is less frequent.
> > > 
> > > We could do that triage in ReduceResourceUsage when we know we're above
the
> > > limit, that way, no need to introduce a client to RP.
> > 
> > As mentioned before, we could probably rebuild the lists in
> > TileManager::AssignGpuMemoryToTiles() instead, which would drop the need for
a
> > client.
> 
> So I gave the idea reveman's suggestion a try but it didn't turn out very
nice.
> This is because if we'd rebuild the unused and unlockable/exported list every
> AssignGpuMemoryToTiles we'd for the most part just build an identical list, as
> we would usually release an already exported resource, and , wasting lots of
> time accomplishing nothing.

hm, I'm not sure I follow what you tried. Why do you have to rebuild a list? I
was thinking that released resources would always go into a
|resources_in_use_by_consumer_| list and once every time AssignGpuMemoryToTiles
is called would we would try moving things from that list to
|unused_resources_|. Maybe even do it lazily when out of unused resources.. The
general idea would be to iterate over resources_in_use_by_consumer_ at most one
time per AssignGpuMemoryToTiles call instead of once per resource allocation.

> 
> I think the best we can do right now is to as piman suggested and only care
> about returned resources when we try to reduce memory and add a resource to
> either a unused list or an exported list when the ResourcePool releases a
> resource. In ReduceResourceUsage, if unused_resources_ is empty, we'd check if
> exported resources are still exported and if not release them as well. Correct
> me if I'm wrong, but this way we shouldn't use CanLockForWrite() but
> InUseByConsumer().

Usage of InUseByConsumer() is fine if it also implies !CanLockForWrite(). I
don't think ReduceResourceUsage is good enough unless I'm missing something. We
shouldn't be allocating new resources if there's a resource not in use by
consumer that we can use.

> 
> However, this way we won't immediately get back "unused" resources (i.e.
> released but exported) immediately after they are returned from the parent
> compositor.

Getting them back when acquiring a new resource but unused_resources_ is empty
is the best we can do. Anything more immediate than that is not going to make a
difference.

[jadahl, 2013-10-28 16:50:00]

On 2013/10/28 16:18:08, David Reveman wrote:
> On 2013/10/28 14:33:48, jadahl wrote:
> > On 2013/10/28 09:59:29, jadahl wrote:
> > >
> https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
> > > File cc/resources/resource_pool.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > cc/resources/resource_pool.cc:43: if (resource_provider_.get())
> > > On 2013/10/25 21:51:11, piman wrote:
> > > > Can this be NULL? Below we SetResourceUsageLimits which will destroy the
> > > > resources, which will deref resource_provider_. So resource_provider_
has
> to
> > > be
> > > > still there. If it's not, there's bigger problems (e.g. we would leak
> > > > resources).
> > > > So I don't think resource_provider_ needs to be a weakptr at all.
> > > 
> > > I couldn't see any clearly structured order of which ResourcePool and
> > > ResourceProvider would be destructed. ResourceProvider is owned by
> > > LayerTreeHostImpl while ResourcePool is owned by TileManager which itself
is
> > > owned by LayerTreeHostImpl. As there was no explicit requirement (as far
as
> I
> > > could see in the code) that ResourceProvider would be destructed after
> > > ResourcePool I added a weak pointer in order not to rely on this implicit
> > > behavior.
> > > 
> > > Anyhow, as regarding reveman's comment from friday, we can probably drop
the
> > > ResourceProviderClient, including the weak pointer, and just update the
list
> > > from ManageTiles before scheduling tasks.
> > > 
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > cc/resources/resource_pool.cc:89: if
> > > (resource_provider_->IsExported(resource->id()))
> > > On 2013/10/25 21:51:11, piman wrote:
> > > > In PrioritizedResourceManager, we use InUseByConsumer(id) && !IsLost(id)
> for
> > > the
> > > > same purpose. Can we use the same pattern in both?
> > > > 
> > > > Actually, InUseByConsumer seems like the right choice anyway, because if
> > it's
> > > > lost, then it's effectively still exported (i.e. destroying the resource
> > won't
> > > > destroy anything until the parent destroys it too, which it won't
because
> > it's
> > > > lost), so maybe we can just use that?
> > > 
> > > As mentioned by reveman we can probably use CanLockForWrite here, sorting
> > > resources in "unused" and "unlockable" resources. This will itself check
if
> > the
> > > resource is exported or not, and we'll cover more cases where
> AcquireResource
> > > would iterate through unlockable resources.
> > > 
> > > How does that sound to you?
> > > 
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > cc/resources/resource_pool.cc:146: void
> > > ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
> > > On 2013/10/25 21:51:11, piman wrote:
> > > > I'm not a big fan of this. We'll pay that cost for each resource on each
> > > frame,
> > > > whereas discriminating exported vs non is only needed when we need to
> > actually
> > > > discard resources, which is less frequent.
> > > > 
> > > > We could do that triage in ReduceResourceUsage when we know we're above
> the
> > > > limit, that way, no need to introduce a client to RP.
> > > 
> > > As mentioned before, we could probably rebuild the lists in
> > > TileManager::AssignGpuMemoryToTiles() instead, which would drop the need
for
> a
> > > client.
> > 
> > So I gave the idea reveman's suggestion a try but it didn't turn out very
> nice.
> > This is because if we'd rebuild the unused and unlockable/exported list
every
> > AssignGpuMemoryToTiles we'd for the most part just build an identical list,
as
> > we would usually release an already exported resource, and , wasting lots of
> > time accomplishing nothing.
> 
> hm, I'm not sure I follow what you tried. Why do you have to rebuild a list? I
> was thinking that released resources would always go into a
> |resources_in_use_by_consumer_| list and once every time
AssignGpuMemoryToTiles
> is called would we would try moving things from that list to
> |unused_resources_|. Maybe even do it lazily when out of unused resources..
The
> general idea would be to iterate over resources_in_use_by_consumer_ at most
one
> time per AssignGpuMemoryToTiles call instead of once per resource allocation.
> 
> > 
> > I think the best we can do right now is to as piman suggested and only care
> > about returned resources when we try to reduce memory and add a resource to
> > either a unused list or an exported list when the ResourcePool releases a
> > resource. In ReduceResourceUsage, if unused_resources_ is empty, we'd check
if
> > exported resources are still exported and if not release them as well.
Correct
> > me if I'm wrong, but this way we shouldn't use CanLockForWrite() but
> > InUseByConsumer().
> 
> Usage of InUseByConsumer() is fine if it also implies !CanLockForWrite(). I
> don't think ReduceResourceUsage is good enough unless I'm missing something.
We
> shouldn't be allocating new resources if there's a resource not in use by
> consumer that we can use.
> 
> > 
> > However, this way we won't immediately get back "unused" resources (i.e.
> > released but exported) immediately after they are returned from the parent
> > compositor.
> 
> Getting them back when acquiring a new resource but unused_resources_ is empty
> is the best we can do. Anything more immediate than that is not going to make
a
> difference.

Not sure that is a good idea as the issue to begin with was that AcquireResource
was iterating through lots of exported resources every time it was called. Could
it instead be checked whenever the delegating renderer receives returned
resources similar to the original approach but using something else than a
ResourceProviderClient?

[reveman, 2013-10-28 16:59:17]

On 2013/10/28 16:50:00, jadahl wrote:
> On 2013/10/28 16:18:08, David Reveman wrote:
> > On 2013/10/28 14:33:48, jadahl wrote:
> > > On 2013/10/28 09:59:29, jadahl wrote:
> > > >
> >
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
> > > > File cc/resources/resource_pool.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > cc/resources/resource_pool.cc:43: if (resource_provider_.get())
> > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > Can this be NULL? Below we SetResourceUsageLimits which will destroy
the
> > > > > resources, which will deref resource_provider_. So resource_provider_
> has
> > to
> > > > be
> > > > > still there. If it's not, there's bigger problems (e.g. we would leak
> > > > > resources).
> > > > > So I don't think resource_provider_ needs to be a weakptr at all.
> > > > 
> > > > I couldn't see any clearly structured order of which ResourcePool and
> > > > ResourceProvider would be destructed. ResourceProvider is owned by
> > > > LayerTreeHostImpl while ResourcePool is owned by TileManager which
itself
> is
> > > > owned by LayerTreeHostImpl. As there was no explicit requirement (as far
> as
> > I
> > > > could see in the code) that ResourceProvider would be destructed after
> > > > ResourcePool I added a weak pointer in order not to rely on this
implicit
> > > > behavior.
> > > > 
> > > > Anyhow, as regarding reveman's comment from friday, we can probably drop
> the
> > > > ResourceProviderClient, including the weak pointer, and just update the
> list
> > > > from ManageTiles before scheduling tasks.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > cc/resources/resource_pool.cc:89: if
> > > > (resource_provider_->IsExported(resource->id()))
> > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > In PrioritizedResourceManager, we use InUseByConsumer(id) &&
!IsLost(id)
> > for
> > > > the
> > > > > same purpose. Can we use the same pattern in both?
> > > > > 
> > > > > Actually, InUseByConsumer seems like the right choice anyway, because
if
> > > it's
> > > > > lost, then it's effectively still exported (i.e. destroying the
resource
> > > won't
> > > > > destroy anything until the parent destroys it too, which it won't
> because
> > > it's
> > > > > lost), so maybe we can just use that?
> > > > 
> > > > As mentioned by reveman we can probably use CanLockForWrite here,
sorting
> > > > resources in "unused" and "unlockable" resources. This will itself check
> if
> > > the
> > > > resource is exported or not, and we'll cover more cases where
> > AcquireResource
> > > > would iterate through unlockable resources.
> > > > 
> > > > How does that sound to you?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > cc/resources/resource_pool.cc:146: void
> > > > ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
> > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > I'm not a big fan of this. We'll pay that cost for each resource on
each
> > > > frame,
> > > > > whereas discriminating exported vs non is only needed when we need to
> > > actually
> > > > > discard resources, which is less frequent.
> > > > > 
> > > > > We could do that triage in ReduceResourceUsage when we know we're
above
> > the
> > > > > limit, that way, no need to introduce a client to RP.
> > > > 
> > > > As mentioned before, we could probably rebuild the lists in
> > > > TileManager::AssignGpuMemoryToTiles() instead, which would drop the need
> for
> > a
> > > > client.
> > > 
> > > So I gave the idea reveman's suggestion a try but it didn't turn out very
> > nice.
> > > This is because if we'd rebuild the unused and unlockable/exported list
> every
> > > AssignGpuMemoryToTiles we'd for the most part just build an identical
list,
> as
> > > we would usually release an already exported resource, and , wasting lots
of
> > > time accomplishing nothing.
> > 
> > hm, I'm not sure I follow what you tried. Why do you have to rebuild a list?
I
> > was thinking that released resources would always go into a
> > |resources_in_use_by_consumer_| list and once every time
> AssignGpuMemoryToTiles
> > is called would we would try moving things from that list to
> > |unused_resources_|. Maybe even do it lazily when out of unused resources..
> The
> > general idea would be to iterate over resources_in_use_by_consumer_ at most
> one
> > time per AssignGpuMemoryToTiles call instead of once per resource
allocation.
> > 
> > > 
> > > I think the best we can do right now is to as piman suggested and only
care
> > > about returned resources when we try to reduce memory and add a resource
to
> > > either a unused list or an exported list when the ResourcePool releases a
> > > resource. In ReduceResourceUsage, if unused_resources_ is empty, we'd
check
> if
> > > exported resources are still exported and if not release them as well.
> Correct
> > > me if I'm wrong, but this way we shouldn't use CanLockForWrite() but
> > > InUseByConsumer().
> > 
> > Usage of InUseByConsumer() is fine if it also implies !CanLockForWrite(). I
> > don't think ReduceResourceUsage is good enough unless I'm missing something.
> We
> > shouldn't be allocating new resources if there's a resource not in use by
> > consumer that we can use.
> > 
> > > 
> > > However, this way we won't immediately get back "unused" resources (i.e.
> > > released but exported) immediately after they are returned from the parent
> > > compositor.
> > 
> > Getting them back when acquiring a new resource but unused_resources_ is
empty
> > is the best we can do. Anything more immediate than that is not going to
make
> a
> > difference.
> 
> Not sure that is a good idea as the issue to begin with was that
AcquireResource
> was iterating through lots of exported resources every time it was called.
Could
> it instead be checked whenever the delegating renderer receives returned
> resources similar to the original approach but using something else than a
> ResourceProviderClient?

What is the problem with the |resources_in_use_by_consumer_| list I suggested
above that we iterate over at most once per call to AssignGpuMemoryToTiles? What
I was trying to explain is that more immediate return of unused resources than
that is meaningless as we're not going to use them anyhow..

[jadahl, 2013-10-28 18:30:49]

On 2013/10/28 16:59:17, David Reveman wrote:
> On 2013/10/28 16:50:00, jadahl wrote:
> > On 2013/10/28 16:18:08, David Reveman wrote:
> > > On 2013/10/28 14:33:48, jadahl wrote:
> > > > On 2013/10/28 09:59:29, jadahl wrote:
> > > > >
> > >
> https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
> > > > > File cc/resources/resource_pool.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > > cc/resources/resource_pool.cc:43: if (resource_provider_.get())
> > > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > > Can this be NULL? Below we SetResourceUsageLimits which will destroy
> the
> > > > > > resources, which will deref resource_provider_. So
resource_provider_
> > has
> > > to
> > > > > be
> > > > > > still there. If it's not, there's bigger problems (e.g. we would
leak
> > > > > > resources).
> > > > > > So I don't think resource_provider_ needs to be a weakptr at all.
> > > > > 
> > > > > I couldn't see any clearly structured order of which ResourcePool and
> > > > > ResourceProvider would be destructed. ResourceProvider is owned by
> > > > > LayerTreeHostImpl while ResourcePool is owned by TileManager which
> itself
> > is
> > > > > owned by LayerTreeHostImpl. As there was no explicit requirement (as
far
> > as
> > > I
> > > > > could see in the code) that ResourceProvider would be destructed after
> > > > > ResourcePool I added a weak pointer in order not to rely on this
> implicit
> > > > > behavior.
> > > > > 
> > > > > Anyhow, as regarding reveman's comment from friday, we can probably
drop
> > the
> > > > > ResourceProviderClient, including the weak pointer, and just update
the
> > list
> > > > > from ManageTiles before scheduling tasks.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > > cc/resources/resource_pool.cc:89: if
> > > > > (resource_provider_->IsExported(resource->id()))
> > > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > > In PrioritizedResourceManager, we use InUseByConsumer(id) &&
> !IsLost(id)
> > > for
> > > > > the
> > > > > > same purpose. Can we use the same pattern in both?
> > > > > > 
> > > > > > Actually, InUseByConsumer seems like the right choice anyway,
because
> if
> > > > it's
> > > > > > lost, then it's effectively still exported (i.e. destroying the
> resource
> > > > won't
> > > > > > destroy anything until the parent destroys it too, which it won't
> > because
> > > > it's
> > > > > > lost), so maybe we can just use that?
> > > > > 
> > > > > As mentioned by reveman we can probably use CanLockForWrite here,
> sorting
> > > > > resources in "unused" and "unlockable" resources. This will itself
check
> > if
> > > > the
> > > > > resource is exported or not, and we'll cover more cases where
> > > AcquireResource
> > > > > would iterate through unlockable resources.
> > > > > 
> > > > > How does that sound to you?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > > cc/resources/resource_pool.cc:146: void
> > > > > ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
> > > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > > I'm not a big fan of this. We'll pay that cost for each resource on
> each
> > > > > frame,
> > > > > > whereas discriminating exported vs non is only needed when we need
to
> > > > actually
> > > > > > discard resources, which is less frequent.
> > > > > > 
> > > > > > We could do that triage in ReduceResourceUsage when we know we're
> above
> > > the
> > > > > > limit, that way, no need to introduce a client to RP.
> > > > > 
> > > > > As mentioned before, we could probably rebuild the lists in
> > > > > TileManager::AssignGpuMemoryToTiles() instead, which would drop the
need
> > for
> > > a
> > > > > client.
> > > > 
> > > > So I gave the idea reveman's suggestion a try but it didn't turn out
very
> > > nice.
> > > > This is because if we'd rebuild the unused and unlockable/exported list
> > every
> > > > AssignGpuMemoryToTiles we'd for the most part just build an identical
> list,
> > as
> > > > we would usually release an already exported resource, and , wasting
lots
> of
> > > > time accomplishing nothing.
> > > 
> > > hm, I'm not sure I follow what you tried. Why do you have to rebuild a
list?
> I
> > > was thinking that released resources would always go into a
> > > |resources_in_use_by_consumer_| list and once every time
> > AssignGpuMemoryToTiles
> > > is called would we would try moving things from that list to
> > > |unused_resources_|. Maybe even do it lazily when out of unused
resources..
> > The
> > > general idea would be to iterate over resources_in_use_by_consumer_ at
most
> > one
> > > time per AssignGpuMemoryToTiles call instead of once per resource
> allocation.
> > > 
> > > > 
> > > > I think the best we can do right now is to as piman suggested and only
> care
> > > > about returned resources when we try to reduce memory and add a resource
> to
> > > > either a unused list or an exported list when the ResourcePool releases
a
> > > > resource. In ReduceResourceUsage, if unused_resources_ is empty, we'd
> check
> > if
> > > > exported resources are still exported and if not release them as well.
> > Correct
> > > > me if I'm wrong, but this way we shouldn't use CanLockForWrite() but
> > > > InUseByConsumer().
> > > 
> > > Usage of InUseByConsumer() is fine if it also implies !CanLockForWrite().
I
> > > don't think ReduceResourceUsage is good enough unless I'm missing
something.
> > We
> > > shouldn't be allocating new resources if there's a resource not in use by
> > > consumer that we can use.
> > > 
> > > > 
> > > > However, this way we won't immediately get back "unused" resources (i.e.
> > > > released but exported) immediately after they are returned from the
parent
> > > > compositor.
> > > 
> > > Getting them back when acquiring a new resource but unused_resources_ is
> empty
> > > is the best we can do. Anything more immediate than that is not going to
> make
> > a
> > > difference.
> > 
> > Not sure that is a good idea as the issue to begin with was that
> AcquireResource
> > was iterating through lots of exported resources every time it was called.
> Could
> > it instead be checked whenever the delegating renderer receives returned
> > resources similar to the original approach but using something else than a
> > ResourceProviderClient?
> 
> What is the problem with the |resources_in_use_by_consumer_| list I suggested
> above that we iterate over at most once per call to AssignGpuMemoryToTiles?
What
> I was trying to explain is that more immediate return of unused resources than
> that is meaningless as we're not going to use them anyhow..

I tried it, and it just regenerated the exact same list that was there every
time. One thing I discovered though, was that resources never seemed to be
returned from the parent compositor (using Android), which seems broken and
could be the reason why the issue with iterating through the unused list was
such a severe performance hit. Is it expected that the parent compositor should
return resources every frame? If not, we should not have to rebuild it every
frame as well, no?

[reveman, 2013-10-28 18:49:57]

On 2013/10/28 18:30:49, jadahl wrote:
> On 2013/10/28 16:59:17, David Reveman wrote:
> > On 2013/10/28 16:50:00, jadahl wrote:
> > > On 2013/10/28 16:18:08, David Reveman wrote:
> > > > On 2013/10/28 14:33:48, jadahl wrote:
> > > > > On 2013/10/28 09:59:29, jadahl wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc
> > > > > > File cc/resources/resource_pool.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > > > cc/resources/resource_pool.cc:43: if (resource_provider_.get())
> > > > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > > > Can this be NULL? Below we SetResourceUsageLimits which will
destroy
> > the
> > > > > > > resources, which will deref resource_provider_. So
> resource_provider_
> > > has
> > > > to
> > > > > > be
> > > > > > > still there. If it's not, there's bigger problems (e.g. we would
> leak
> > > > > > > resources).
> > > > > > > So I don't think resource_provider_ needs to be a weakptr at all.
> > > > > > 
> > > > > > I couldn't see any clearly structured order of which ResourcePool
and
> > > > > > ResourceProvider would be destructed. ResourceProvider is owned by
> > > > > > LayerTreeHostImpl while ResourcePool is owned by TileManager which
> > itself
> > > is
> > > > > > owned by LayerTreeHostImpl. As there was no explicit requirement (as
> far
> > > as
> > > > I
> > > > > > could see in the code) that ResourceProvider would be destructed
after
> > > > > > ResourcePool I added a weak pointer in order not to rely on this
> > implicit
> > > > > > behavior.
> > > > > > 
> > > > > > Anyhow, as regarding reveman's comment from friday, we can probably
> drop
> > > the
> > > > > > ResourceProviderClient, including the weak pointer, and just update
> the
> > > list
> > > > > > from ManageTiles before scheduling tasks.
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > > > cc/resources/resource_pool.cc:89: if
> > > > > > (resource_provider_->IsExported(resource->id()))
> > > > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > > > In PrioritizedResourceManager, we use InUseByConsumer(id) &&
> > !IsLost(id)
> > > > for
> > > > > > the
> > > > > > > same purpose. Can we use the same pattern in both?
> > > > > > > 
> > > > > > > Actually, InUseByConsumer seems like the right choice anyway,
> because
> > if
> > > > > it's
> > > > > > > lost, then it's effectively still exported (i.e. destroying the
> > resource
> > > > > won't
> > > > > > > destroy anything until the parent destroys it too, which it won't
> > > because
> > > > > it's
> > > > > > > lost), so maybe we can just use that?
> > > > > > 
> > > > > > As mentioned by reveman we can probably use CanLockForWrite here,
> > sorting
> > > > > > resources in "unused" and "unlockable" resources. This will itself
> check
> > > if
> > > > > the
> > > > > > resource is exported or not, and we'll cover more cases where
> > > > AcquireResource
> > > > > > would iterate through unlockable resources.
> > > > > > 
> > > > > > How does that sound to you?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/43753002/diff/1/cc/resources/resource_pool.cc...
> > > > > > cc/resources/resource_pool.cc:146: void
> > > > > > ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {
> > > > > > On 2013/10/25 21:51:11, piman wrote:
> > > > > > > I'm not a big fan of this. We'll pay that cost for each resource
on
> > each
> > > > > > frame,
> > > > > > > whereas discriminating exported vs non is only needed when we need
> to
> > > > > actually
> > > > > > > discard resources, which is less frequent.
> > > > > > > 
> > > > > > > We could do that triage in ReduceResourceUsage when we know we're
> > above
> > > > the
> > > > > > > limit, that way, no need to introduce a client to RP.
> > > > > > 
> > > > > > As mentioned before, we could probably rebuild the lists in
> > > > > > TileManager::AssignGpuMemoryToTiles() instead, which would drop the
> need
> > > for
> > > > a
> > > > > > client.
> > > > > 
> > > > > So I gave the idea reveman's suggestion a try but it didn't turn out
> very
> > > > nice.
> > > > > This is because if we'd rebuild the unused and unlockable/exported
list
> > > every
> > > > > AssignGpuMemoryToTiles we'd for the most part just build an identical
> > list,
> > > as
> > > > > we would usually release an already exported resource, and , wasting
> lots
> > of
> > > > > time accomplishing nothing.
> > > > 
> > > > hm, I'm not sure I follow what you tried. Why do you have to rebuild a
> list?
> > I
> > > > was thinking that released resources would always go into a
> > > > |resources_in_use_by_consumer_| list and once every time
> > > AssignGpuMemoryToTiles
> > > > is called would we would try moving things from that list to
> > > > |unused_resources_|. Maybe even do it lazily when out of unused
> resources..
> > > The
> > > > general idea would be to iterate over resources_in_use_by_consumer_ at
> most
> > > one
> > > > time per AssignGpuMemoryToTiles call instead of once per resource
> > allocation.
> > > > 
> > > > > 
> > > > > I think the best we can do right now is to as piman suggested and only
> > care
> > > > > about returned resources when we try to reduce memory and add a
resource
> > to
> > > > > either a unused list or an exported list when the ResourcePool
releases
> a
> > > > > resource. In ReduceResourceUsage, if unused_resources_ is empty, we'd
> > check
> > > if
> > > > > exported resources are still exported and if not release them as well.
> > > Correct
> > > > > me if I'm wrong, but this way we shouldn't use CanLockForWrite() but
> > > > > InUseByConsumer().
> > > > 
> > > > Usage of InUseByConsumer() is fine if it also implies
!CanLockForWrite().
> I
> > > > don't think ReduceResourceUsage is good enough unless I'm missing
> something.
> > > We
> > > > shouldn't be allocating new resources if there's a resource not in use
by
> > > > consumer that we can use.
> > > > 
> > > > > 
> > > > > However, this way we won't immediately get back "unused" resources
(i.e.
> > > > > released but exported) immediately after they are returned from the
> parent
> > > > > compositor.
> > > > 
> > > > Getting them back when acquiring a new resource but unused_resources_ is
> > empty
> > > > is the best we can do. Anything more immediate than that is not going to
> > make
> > > a
> > > > difference.
> > > 
> > > Not sure that is a good idea as the issue to begin with was that
> > AcquireResource
> > > was iterating through lots of exported resources every time it was called.
> > Could
> > > it instead be checked whenever the delegating renderer receives returned
> > > resources similar to the original approach but using something else than a
> > > ResourceProviderClient?
> > 
> > What is the problem with the |resources_in_use_by_consumer_| list I
suggested
> > above that we iterate over at most once per call to AssignGpuMemoryToTiles?
> What
> > I was trying to explain is that more immediate return of unused resources
than
> > that is meaningless as we're not going to use them anyhow..
> 
> I tried it, and it just regenerated the exact same list that was there every
> time. One thing I discovered though, was that resources never seemed to be
> returned from the parent compositor (using Android), which seems broken and
> could be the reason why the issue with iterating through the unused list was
> such a severe performance hit. Is it expected that the parent compositor
should
> return resources every frame? If not, we should not have to rebuild it every
> frame as well, no?

How large does the in_use_by_consumer list become? This list should not be able
to grow very large. I'd assume that resources are returned at about the same
rate that we initialize new resources.

Assuming that there is indeed a problem with iterating over all resources in
ResourcePool::AcquireResource then iterating over only the resources that are in
use by consumer once per call to AssignGpuMemoryToTiles should be a major
improvement. Are you saying that this is not enough?

[jadahl, 2013-10-29 10:56:07]

On 2013/10/28 18:49:57, David Reveman wrote:
> On 2013/10/28 18:30:49, jadahl wrote:
> > On 2013/10/28 16:59:17, David Reveman wrote:
> > > On 2013/10/28 16:50:00, jadahl wrote:
> > > > On 2013/10/28 16:18:08, David Reveman wrote:
> > > > > 
> > > > > Getting them back when acquiring a new resource but unused_resources_
is
> > > empty
> > > > > is the best we can do. Anything more immediate than that is not going
to
> > > make
> > > > a
> > > > > difference.
> > > > 
> > > > Not sure that is a good idea as the issue to begin with was that
> > > AcquireResource
> > > > was iterating through lots of exported resources every time it was
called.
> > > Could
> > > > it instead be checked whenever the delegating renderer receives returned
> > > > resources similar to the original approach but using something else than
a
> > > > ResourceProviderClient?
> > > 
> > > What is the problem with the |resources_in_use_by_consumer_| list I
> suggested
> > > above that we iterate over at most once per call to
AssignGpuMemoryToTiles?
> > What
> > > I was trying to explain is that more immediate return of unused resources
> than
> > > that is meaningless as we're not going to use them anyhow..
> > 
> > I tried it, and it just regenerated the exact same list that was there every
> > time. One thing I discovered though, was that resources never seemed to be
> > returned from the parent compositor (using Android), which seems broken and
> > could be the reason why the issue with iterating through the unused list was
> > such a severe performance hit. Is it expected that the parent compositor
> should
> > return resources every frame? If not, we should not have to rebuild it every
> > frame as well, no?
> 
> How large does the in_use_by_consumer list become? This list should not be
able
> to grow very large. I'd assume that resources are returned at about the same
> rate that we initialize new resources.

Over 600 elements, but I think the size I observed is another bug in itself as
returning of resources seems broken on at least Android (ContentShell).

> 
> Assuming that there is indeed a problem with iterating over all resources in
> ResourcePool::AcquireResource then iterating over only the resources that are
in
> use by consumer once per call to AssignGpuMemoryToTiles should be a major
> improvement. Are you saying that this is not enough?

It fixes the issue of the stalling time growing exponentially, but still seemed
like a waste of CPU time as the list rebuild didn't change the resulting list.
However, as discovered, this was mostly because resources are not returned. If
no resource is returned between two frames, then the unused/exported list
rebuild is unnecessary. If it is expected to have resources return between every
frame, then this is not a problem.

So, my idea of the issue now is: if resources are expected to be returned
between every frame, we can rebuild the lists in AssignGpuMemoryToTiles and it
would solve the problem of avoiding iterating through exported resources in
AcquireResource. If resources are not being expected to return every frame and
there are several frames without resources being returned, we should only
rebuild the list when needed, i.e. when resources are returned. I'm yet to have
a clear understanding of the flow of resource consumption by the compositor pair
so I'd appreciate if this could be clarified.

[reveman, 2013-10-29 13:22:43]

On 2013/10/29 10:56:07, jadahl wrote:
> On 2013/10/28 18:49:57, David Reveman wrote:
> > On 2013/10/28 18:30:49, jadahl wrote:
> > > On 2013/10/28 16:59:17, David Reveman wrote:
> > > > On 2013/10/28 16:50:00, jadahl wrote:
> > > > > On 2013/10/28 16:18:08, David Reveman wrote:
> > > > > > 
> > > > > > Getting them back when acquiring a new resource but
unused_resources_
> is
> > > > empty
> > > > > > is the best we can do. Anything more immediate than that is not
going
> to
> > > > make
> > > > > a
> > > > > > difference.
> > > > > 
> > > > > Not sure that is a good idea as the issue to begin with was that
> > > > AcquireResource
> > > > > was iterating through lots of exported resources every time it was
> called.
> > > > Could
> > > > > it instead be checked whenever the delegating renderer receives
returned
> > > > > resources similar to the original approach but using something else
than
> a
> > > > > ResourceProviderClient?
> > > > 
> > > > What is the problem with the |resources_in_use_by_consumer_| list I
> > suggested
> > > > above that we iterate over at most once per call to
> AssignGpuMemoryToTiles?
> > > What
> > > > I was trying to explain is that more immediate return of unused
resources
> > than
> > > > that is meaningless as we're not going to use them anyhow..
> > > 
> > > I tried it, and it just regenerated the exact same list that was there
every
> > > time. One thing I discovered though, was that resources never seemed to be
> > > returned from the parent compositor (using Android), which seems broken
and
> > > could be the reason why the issue with iterating through the unused list
was
> > > such a severe performance hit. Is it expected that the parent compositor
> > should
> > > return resources every frame? If not, we should not have to rebuild it
every
> > > frame as well, no?
> > 
> > How large does the in_use_by_consumer list become? This list should not be
> able
> > to grow very large. I'd assume that resources are returned at about the same
> > rate that we initialize new resources.
> 
> Over 600 elements, but I think the size I observed is another bug in itself as
> returning of resources seems broken on at least Android (ContentShell).

Yes, 600 resources in use by the consumer seems like a bug.

> 
> > 
> > Assuming that there is indeed a problem with iterating over all resources in
> > ResourcePool::AcquireResource then iterating over only the resources that
are
> in
> > use by consumer once per call to AssignGpuMemoryToTiles should be a major
> > improvement. Are you saying that this is not enough?
> 
> It fixes the issue of the stalling time growing exponentially, but still
seemed
> like a waste of CPU time as the list rebuild didn't change the resulting list.
> However, as discovered, this was mostly because resources are not returned. If
> no resource is returned between two frames, then the unused/exported list
> rebuild is unnecessary. If it is expected to have resources return between
every
> frame, then this is not a problem.
> 
> So, my idea of the issue now is: if resources are expected to be returned
> between every frame, we can rebuild the lists in AssignGpuMemoryToTiles and it
> would solve the problem of avoiding iterating through exported resources in
> AcquireResource. If resources are not being expected to return every frame and
> there are several frames without resources being returned, we should only
> rebuild the list when needed, i.e. when resources are returned. I'm yet to
have
> a clear understanding of the flow of resource consumption by the compositor
pair
> so I'd appreciate if this could be clarified.

Sounds good. Lets figure out why we have so many unused resources that are in
use by the consumer first.

[jadahl, 2013-10-30 13:54:12]

Uploaded a new version.

[reveman, 2013-10-30 14:56:01]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:51: if
(!resource_provider_->CanLockForWrite(resource->id()))
Do we still need this? What's the difference between CanLockForWrite and
InUseByConsumer? I'd prefer if the resource pool didn't have to be aware of both
these concepts unless absolutely necessary.

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:81: return;
This destroys the resource. Are we allowed to do that while InUseByConsumer
returns true? If we are, should we consider these resources in
ResourcePool::ReduceResourceUsage() too?

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:86: if
(resource_provider_->InUseByConsumer(resource->id()))
Can we remove this check and always add released resources to
|consumed_resources_|? I'd prefer to only check InUseByConsumer in one place.

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:136: while (it != consumed_resources_.end()) {
nit: consider using a temporary "Resource* resource = *it;" here. that's more
consistent with other loops in this file.

[danakj, 2013-10-30 15:12:40]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:86: if
(resource_provider_->InUseByConsumer(resource->id()))
On 2013/10/30 14:56:02, David Reveman wrote:
> Can we remove this check and always add released resources to
> |consumed_resources_|? I'd prefer to only check InUseByConsumer in one place.

Yes you can RP holds onto it until it comes back from the parent before really
deleting it.

[reveman, 2013-10-30 18:50:46]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:86: if
(resource_provider_->InUseByConsumer(resource->id()))
On 2013/10/30 15:12:40, danakj wrote:
> On 2013/10/30 14:56:02, David Reveman wrote:
> > Can we remove this check and always add released resources to
> > |consumed_resources_|? I'd prefer to only check InUseByConsumer in one
place.
> 
> Yes you can RP holds onto it until it comes back from the parent before really
> deleting it. 

I assume this is a reply to the above comment about destroying resources
currently in use by consumer. I'm not sure it's a good idea to allow these
resources to be removed from the pool if destruction is delayed as it would
cause the memory usage accounting here to be off.

[danakj, 2013-10-30 18:52:55]

On Wed, Oct 30, 2013 at 2:50 PM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.**org/43753002/diff/230001/cc/**
>
resources/resource_pool.cc<https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_pool.cc>
> File cc/resources/resource_pool.cc (right):
>
> https://codereview.chromium.**org/43753002/diff/230001/cc/**
>
resources/resource_pool.cc#**newcode86<https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_pool.cc#newcode86>
> cc/resources/resource_pool.cc:**86: if
> (resource_provider_->**InUseByConsumer(resource->id()**))
> On 2013/10/30 15:12:40, danakj wrote:
>
>> On 2013/10/30 14:56:02, David Reveman wrote:
>> > Can we remove this check and always add released resources to
>> > |consumed_resources_|? I'd prefer to only check InUseByConsumer in
>>
> one place.
>
>  Yes you can RP holds onto it until it comes back from the parent
>>
> before really
>
>> deleting it.
>>
>
> I assume this is a reply to the above comment about destroying resources
> currently in use by consumer. I'm not sure it's a good idea to allow
> these resources to be removed from the pool if destruction is delayed as
> it would cause the memory usage accounting here to be off.
>

Yep, and that is a good point. In PrioritizedResourceManager we prefer to
not evict exported things, since they won't actually free any memory.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2013-10-30 19:35:30]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:51: if
(!resource_provider_->CanLockForWrite(resource->id()))
On 2013/10/30 14:56:02, David Reveman wrote:
> Do we still need this? What's the difference between CanLockForWrite and
> InUseByConsumer? I'd prefer if the resource pool didn't have to be aware of
both
> these concepts unless absolutely necessary.

There's stronger things needed for CanLockForWrite, such as:
- it's not an external texture (shouldn't matter here)
- it's not already locked (shouldn't happen either)
- the "read lock fence" has passed, which matters here (otherwise we risk
out-of-order writes), but shouldn't matter for memory management.

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:81: return;
On 2013/10/30 14:56:02, David Reveman wrote:
> This destroys the resource. Are we allowed to do that while InUseByConsumer
> returns true?

It's absolutely legal (the RP holds on to the texture until it's returned), but
it won't actually free any memory until InUseByConsumer becomes false.

> If we are, should we consider these resources in
> ResourcePool::ReduceResourceUsage() too?

https://codereview.chromium.org/43753002/diff/230001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:598: resource_pool_->CheckConsumedResources();
note, if this becomes to expensive to do all the time, the resource return path
goes through the LTHI which could forward a signal "we may have returned some
resources" to here.

[reveman, 2013-10-30 21:10:28]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:51: if
(!resource_provider_->CanLockForWrite(resource->id()))
On 2013/10/30 19:35:30, piman wrote:
> On 2013/10/30 14:56:02, David Reveman wrote:
> > Do we still need this? What's the difference between CanLockForWrite and
> > InUseByConsumer? I'd prefer if the resource pool didn't have to be aware of
> both
> > these concepts unless absolutely necessary.
> 
> There's stronger things needed for CanLockForWrite, such as:
> - it's not an external texture (shouldn't matter here)
> - it's not already locked (shouldn't happen either)
> - the "read lock fence" has passed, which matters here (otherwise we risk
> out-of-order writes), but shouldn't matter for memory management.

Ok, thanks for explaining. Is there a reason we need to use both though? If
InUSeByConsumer implies !CanLockForWrite then we should be able to use just
CanLockForWrite in CheckConsumedResources() and get rid of this call here. That
seems better from a memory management perspective too. If we can't lock for
write, then memory is likely not going to be freed if we destroy the resource.

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:81: return;
On 2013/10/30 19:35:30, piman wrote:
> On 2013/10/30 14:56:02, David Reveman wrote:
> > This destroys the resource. Are we allowed to do that while InUseByConsumer
> > returns true?
> 
> It's absolutely legal (the RP holds on to the texture until it's returned),
but
> it won't actually free any memory until InUseByConsumer becomes false.

Got it. I don't think we should be freeing the memory in this case but instead
wait for it to become !InUseByConsumer/CanLockForWrite.

> 
> > If we are, should we consider these resources in
> > ResourcePool::ReduceResourceUsage() too?

https://codereview.chromium.org/43753002/diff/230001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:598: resource_pool_->CheckConsumedResources();
On 2013/10/30 19:35:30, piman wrote:
> note, if this becomes to expensive to do all the time, the resource return
path
> goes through the LTHI which could forward a signal "we may have returned some
> resources" to here.

I think this is good enough for now but a comment that points out what piman@
mentioned would be nice.

[piman, 2013-10-30 21:41:11]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:51: if
(!resource_provider_->CanLockForWrite(resource->id()))
On 2013/10/30 21:10:28, David Reveman wrote:
> On 2013/10/30 19:35:30, piman wrote:
> > On 2013/10/30 14:56:02, David Reveman wrote:
> > > Do we still need this? What's the difference between CanLockForWrite and
> > > InUseByConsumer? I'd prefer if the resource pool didn't have to be aware
of
> > both
> > > these concepts unless absolutely necessary.
> > 
> > There's stronger things needed for CanLockForWrite, such as:
> > - it's not an external texture (shouldn't matter here)
> > - it's not already locked (shouldn't happen either)
> > - the "read lock fence" has passed, which matters here (otherwise we risk
> > out-of-order writes), but shouldn't matter for memory management.
> 
> Ok, thanks for explaining. Is there a reason we need to use both though? If
> InUSeByConsumer implies !CanLockForWrite then we should be able to use just
> CanLockForWrite in CheckConsumedResources() and get rid of this call here.
That
> seems better from a memory management perspective too. If we can't lock for
> write, then memory is likely not going to be freed if we destroy the resource.

My take is that from what we control in Chrome, they are different. We count
memory wrt the limit at what we give to the driver, and we generally don't try
to second-guess it. So, even though the fence isn't passed, we can still
glDeleteTexture it and count it as freed immediately, for the same reason that
when creating the texture we count the memory as in-use immediately, even though
the actual allocation may be pipelined inside the driver, so I don't think we
should be trying to be too smart.

That said, I don't feel that strongly. If it simplifies code otherwise to use
CanLockForWrite everywhere, I'm ok with it.

[reveman, 2013-10-30 21:48:20]

On 2013/10/30 21:41:11, piman wrote:
>
https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
> File cc/resources/resource_pool.cc (right):
> 
>
https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
> cc/resources/resource_pool.cc:51: if
> (!resource_provider_->CanLockForWrite(resource->id()))
> On 2013/10/30 21:10:28, David Reveman wrote:
> > On 2013/10/30 19:35:30, piman wrote:
> > > On 2013/10/30 14:56:02, David Reveman wrote:
> > > > Do we still need this? What's the difference between CanLockForWrite and
> > > > InUseByConsumer? I'd prefer if the resource pool didn't have to be aware
> of
> > > both
> > > > these concepts unless absolutely necessary.
> > > 
> > > There's stronger things needed for CanLockForWrite, such as:
> > > - it's not an external texture (shouldn't matter here)
> > > - it's not already locked (shouldn't happen either)
> > > - the "read lock fence" has passed, which matters here (otherwise we risk
> > > out-of-order writes), but shouldn't matter for memory management.
> > 
> > Ok, thanks for explaining. Is there a reason we need to use both though? If
> > InUSeByConsumer implies !CanLockForWrite then we should be able to use just
> > CanLockForWrite in CheckConsumedResources() and get rid of this call here.
> That
> > seems better from a memory management perspective too. If we can't lock for
> > write, then memory is likely not going to be freed if we destroy the
resource.
> 
> My take is that from what we control in Chrome, they are different. We count
> memory wrt the limit at what we give to the driver, and we generally don't try
> to second-guess it. So, even though the fence isn't passed, we can still
> glDeleteTexture it and count it as freed immediately, for the same reason that
> when creating the texture we count the memory as in-use immediately, even
though
> the actual allocation may be pipelined inside the driver, so I don't think we
> should be trying to be too smart.

OK, makes sense.

> 
> That said, I don't feel that strongly. If it simplifies code otherwise to use
> CanLockForWrite everywhere, I'm ok with it.

I don't feel that strongly either. The reasoning above justifies using both some
I'm fine with that.

[jadahl, 2013-10-31 08:38:41]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:81: return;
On 2013/10/30 19:35:30, piman wrote:
> On 2013/10/30 14:56:02, David Reveman wrote:
> > This destroys the resource. Are we allowed to do that while InUseByConsumer
> > returns true?
> 
> It's absolutely legal (the RP holds on to the texture until it's returned),
but
> it won't actually free any memory until InUseByConsumer becomes false.

How about deleting resources where |lock_for_read_count_| is non-zero? Looks
like we'd both hit an DCHECK in DeleteResource and the Unlock would crash. It
also looks like |lock_for_read_count_| will never be be non-zero here because no
user of the API keeps the lock around this way, but the API seems to allow it.

Should that be taken into consideration?

> Got it. I don't think we should be freeing the memory in this case
> but instead wait for it to become !InUseByConsumer/CanLockForWrite.

Agreed.

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:86: if
(resource_provider_->InUseByConsumer(resource->id()))
On 2013/10/30 14:56:02, David Reveman wrote:
> Can we remove this check and always add released resources to
> |consumed_resources_|? I'd prefer to only check InUseByConsumer in one place.

As far as I can see, this should be fine, as we always rebuild the list before
starting to schedule new tasks.

[danakj, 2013-10-31 17:27:51]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:81: return;
On 2013/10/31 08:38:42, jadahl wrote:
> On 2013/10/30 19:35:30, piman wrote:
> > On 2013/10/30 14:56:02, David Reveman wrote:
> > > This destroys the resource. Are we allowed to do that while
InUseByConsumer
> > > returns true?
> > 
> > It's absolutely legal (the RP holds on to the texture until it's returned),
> but
> > it won't actually free any memory until InUseByConsumer becomes false.
> 
> How about deleting resources where |lock_for_read_count_| is non-zero? Looks
> like we'd both hit an DCHECK in DeleteResource and the Unlock would crash. It
> also looks like |lock_for_read_count_| will never be be non-zero here because
no
> user of the API keeps the lock around this way, but the API seems to allow it.
> 
> Should that be taken into consideration?

Generally we should lock resources only while they are being actively used, so
you should expect nothing to have read locks on it here, and you should be
tracking separately if the resource is in use or not. For instance, if we're
using the resource to draw to the screen, we'll only take the lock briefly while
drawing that resource, not hold it for as long as we may draw with it at some
point. Using the lock for read count here seems like the wrong thing to do.

> > Got it. I don't think we should be freeing the memory in this case
> > but instead wait for it to become !InUseByConsumer/CanLockForWrite.
> 
> Agreed.

[reveman, 2013-10-31 19:41:48]

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:81: return;
On 2013/10/31 17:27:52, danakj wrote:
> On 2013/10/31 08:38:42, jadahl wrote:
> > On 2013/10/30 19:35:30, piman wrote:
> > > On 2013/10/30 14:56:02, David Reveman wrote:
> > > > This destroys the resource. Are we allowed to do that while
> InUseByConsumer
> > > > returns true?
> > > 
> > > It's absolutely legal (the RP holds on to the texture until it's
returned),
> > but
> > > it won't actually free any memory until InUseByConsumer becomes false.
> > 
> > How about deleting resources where |lock_for_read_count_| is non-zero? Looks
> > like we'd both hit an DCHECK in DeleteResource and the Unlock would crash.
It
> > also looks like |lock_for_read_count_| will never be be non-zero here
because
> no
> > user of the API keeps the lock around this way, but the API seems to allow
it.
> > 
> > Should that be taken into consideration?
> 
> Generally we should lock resources only while they are being actively used, so
> you should expect nothing to have read locks on it here, and you should be
> tracking separately if the resource is in use or not. For instance, if we're
> using the resource to draw to the screen, we'll only take the lock briefly
while
> drawing that resource, not hold it for as long as we may draw with it at some
> point. Using the lock for read count here seems like the wrong thing to do.

The read locks are supposed to protect against usage of resources by the GPU. It
needs to be safe to map and modify the actual memory used by the texture without
having to worry about the GPU reading from it once CanLockForWrite is true. We
don't have proper synchronization primitives in place to track this accurately
at the moment but it's something we intend to add. The read locks are currently
based on assumptions about pipeline depth but crucial for 0-copy and async
uploads to work on Android.

> 
> > > Got it. I don't think we should be freeing the memory in this case
> > > but instead wait for it to become !InUseByConsumer/CanLockForWrite.
> > 
> > Agreed.
>

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:84: unused_memory_usage_bytes_ +=
resource->bytes();
This needs to be moved to CheckConsumedResources() instead.

https://codereview.chromium.org/43753002/diff/230001/cc/resources/resource_po...
cc/resources/resource_pool.cc:86: if
(resource_provider_->InUseByConsumer(resource->id()))
On 2013/10/31 08:38:42, jadahl wrote:
> On 2013/10/30 14:56:02, David Reveman wrote:
> > Can we remove this check and always add released resources to
> > |consumed_resources_|? I'd prefer to only check InUseByConsumer in one
place.
> 
> As far as I can see, this should be fine, as we always rebuild the list before
> starting to schedule new tasks.
> 

Yes, let's remove the ResourceUsageTooHigh() code above and always add the
resource to consumed_resources_ here.

[jadahl, 2013-11-01 10:02:49]

New version uploaded.

[reveman, 2013-11-01 15:59:39]

lgtm with nit

https://codereview.chromium.org/43753002/diff/370001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/370001/cc/resources/resource_po...
cc/resources/resource_pool.cc:136: ReduceResourceUsage();
nit: I don't think we need to call ReduceResourceUsage() from here as the tile
manager will always do this explicitly when needed.

[jadahl, 2013-11-02 11:25:54]

New version with nitpick addressed.

Related practical questions: to change the final commit message, do I just
change the description of this code review? You should be able to see it if you
can see the actual git log of this review, but not sure that is possible. Also,
should I fill in some bug number? New one or the üc umbrella bug?

[reveman, 2013-11-02 13:31:13]

another nit.

just change the description to have the commit message change. please add a bug
number if you have one.

https://codereview.chromium.org/43753002/diff/420001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (left):

https://codereview.chromium.org/43753002/diff/420001/cc/resources/resource_po...
cc/resources/resource_pool.cc:51: if
(!resource_provider_->CanLockForWrite(resource->id()))
nit: instead of removing this completely change it to
DCHECK(resource_provider_->CanLockForWrite(resource->id())

[jadahl, 2013-11-02 15:02:30]

Uploaded new, with added DCHECK.

[reveman, 2013-11-03 14:09:12]

lgtm

[jadahl, 2013-11-05 10:34:34]

On 2013/11/03 14:09:12, David Reveman wrote:
> lgtm

Should I wait for lgtm from other reviewers? Contribution docs are a bit unclear
about this when there are abstract ownership in the OWNERS file.

[reveman, 2013-11-05 15:38:26]

You can go ahead and land this after moving "unused_memory_usage_bytes_ +=" to
CheckBusyResources().

https://codereview.chromium.org/43753002/diff/460001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/43753002/diff/460001/cc/resources/resource_po...
cc/resources/resource_pool.cc:77: unused_memory_usage_bytes_ +=
resource->bytes();
This needs to be moved to CheckBusyResources() as only resources that can be
freed should count against unused_memory_usage_bytes_.

[commit-bot: I haz the power, 2013-11-05 16:15:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jadahl@opera.com/43753002/530001

[jadahl, 2013-11-05 16:16:13]

On 2013/11/05 15:38:26, David Reveman wrote:
> You can go ahead and land this after moving "unused_memory_usage_bytes_ +=" to
> CheckBusyResources().
> 

Done, and thanks.

[commit-bot: I haz the power, 2013-11-05 16:34:56]

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_rel&...

[jadahl, 2013-11-06 09:37:22]

On 2013/11/05 16:34:56, I haz the power (commit-bot) wrote:
> 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_rel&...

The update error seems unrelated, and the FAQ mentions nothing of it. The other
fails seems to be unrelated tests failing. Should I just check the commit box
and try again?

[commit-bot: I haz the power, 2013-11-06 13:31:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jadahl@opera.com/43753002/530001

[reveman, 2013-11-06 13:31:18]

On 2013/11/06 09:37:22, jadahl wrote:
> On 2013/11/05 16:34:56, I haz the power (commit-bot) wrote:
> > 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_rel&...
> 
> The update error seems unrelated, and the FAQ mentions nothing of it. The
other
> fails seems to be unrelated tests failing. Should I just check the commit box
> and try again?

Yes, I checked it for you.

[commit-bot: I haz the power, 2013-11-06 17:06:53]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 17:11:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jadahl@opera.com/43753002/530001

[commit-bot: I haz the power, 2013-11-06 21:30:20]

Change committed as 233374