cc: Keep track of busy resources in ResourcePool

cc/resources/resource_pool.cc

Index: cc/resources/resource_pool.cc
diff --git a/cc/resources/resource_pool.cc b/cc/resources/resource_pool.cc
index cef86ded1788d770100d996a1b2d2afc3de42718..d8124eb42ee31c4fe7a5d924c0783c433a4a43d4 100644
--- a/cc/resources/resource_pool.cc
+++ b/cc/resources/resource_pool.cc
@@ -29,16 +29,19 @@ ResourcePool::Resource::~Resource() {
 }
 
 ResourcePool::ResourcePool(ResourceProvider* resource_provider)
-    : resource_provider_(resource_provider),
+    : resource_provider_(resource_provider->AsWeakPtr()),
       max_memory_usage_bytes_(0),
       max_unused_memory_usage_bytes_(0),
       max_resource_count_(0),
       memory_usage_bytes_(0),
       unused_memory_usage_bytes_(0),
       resource_count_(0) {
+  resource_provider_->SetClient(this);
 }
 
 ResourcePool::~ResourcePool() {
+  if (resource_provider_.get())

[piman, 2013/10/25 21:51:11]

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.

[jadahl, 2013/10/28 09:59:30]

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.

+    resource_provider_->SetClient(NULL);
   SetResourceUsageLimits(0, 0, 0);
 }
 
@@ -61,7 +64,7 @@ scoped_ptr<ResourcePool::Resource> ResourcePool::AcquireResource(
   }
 
   // Create new resource.
-  Resource* resource = new Resource(resource_provider_, size, format);
+  Resource* resource = new Resource(resource_provider_.get(), size, format);
 
   // Extend all read locks on all resources until the resource is
   // finished being used, such that we know when resources are
@@ -82,7 +85,11 @@ void ResourcePool::ReleaseResource(
   }
 
   unused_memory_usage_bytes_ += resource->bytes();
-  unused_resources_.push_back(resource.release());
+
+  if (resource_provider_->IsExported(resource->id()))

[piman, 2013/10/25 21:51:11]

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?

[jadahl, 2013/10/28 09:59:30]

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?

+    exported_resources_.push_back(resource.release());
+  else
+    unused_resources_.push_back(resource.release());
 }
 
 void ResourcePool::SetResourceUsageLimits(
@@ -97,7 +104,7 @@ void ResourcePool::SetResourceUsageLimits(
 }
 
 void ResourcePool::ReduceResourceUsage() {
-  while (!unused_resources_.empty()) {
+  while (!unused_resources_.empty() && !exported_resources_.empty()) {
     if (!ResourceUsageTooHigh())
       break;
 
@@ -108,8 +115,17 @@ void ResourcePool::ReduceResourceUsage() {
     // can't be locked for write might also not be truly free-able.
     // We can free the resource here but it doesn't mean that the
     // memory is necessarily returned to the OS.
-    Resource* resource = unused_resources_.front();
-    unused_resources_.pop_front();
+    //
+    // Start with non-exported resources first, as they are more likely
+    // to actually free up memory.
+    Resource* resource;
+    if (unused_resources_.empty()) {
+      resource = exported_resources_.front();
+      exported_resources_.pop_front();
+    } else {
+      resource = unused_resources_.front();
+      unused_resources_.pop_front();
+    }
     memory_usage_bytes_ -= resource->bytes();
     unused_memory_usage_bytes_ -= resource->bytes();
     --resource_count_;
@@ -127,4 +143,28 @@ bool ResourcePool::ResourceUsageTooHigh() {
   return false;
 }
 
+void ResourcePool::ResourceExported(ResourceProvider::ResourceId id) {

[piman, 2013/10/25 21:51:11]

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.

[jadahl, 2013/10/28 09:59:30]

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

+  for (ResourceList::iterator it = unused_resources_.begin();
+       it != unused_resources_.end();
+       ++it) {
+    if ((*it)->id() == id) {
+      exported_resources_.push_back(*it);
+      unused_resources_.erase(it);
+      break;
+    }
+  }
+}
+
+void ResourcePool::ResourceReturned(ResourceProvider::ResourceId id) {
+  for (ResourceList::iterator it = unused_resources_.begin();
+       it != unused_resources_.end();
+       ++it) {
+    if ((*it)->id() == id) {
+      unused_resources_.push_back(*it);
+      exported_resources_.erase(it);
+      break;
+    }
+  }
+}
+
 }  // namespace cc