cc: Keep track of busy resources in ResourcePool

cc/resources/resource_pool.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/resources/resource_pool.h"
 
 #include "cc/resources/resource_provider.h"
 
 namespace cc {
 
@@ skipping 11 matching lines @@
   DCHECK(id());
 }
 
 ResourcePool::Resource::~Resource() {
   DCHECK(id());
   DCHECK(resource_provider_);
   resource_provider_->DeleteResource(id());
 }
 
 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);
 }
 
 scoped_ptr<ResourcePool::Resource> ResourcePool::AcquireResource(
     gfx::Size size, ResourceFormat format) {
   for (ResourceList::iterator it = unused_resources_.begin();
        it != unused_resources_.end(); ++it) {
     Resource* resource = *it;
 
     if (!resource_provider_->CanLockForWrite(resource->id()))
       continue;
     if (resource->size() != size)
       continue;
     if (resource->format() != format)
       continue;
 
     unused_resources_.erase(it);
     unused_memory_usage_bytes_ -= resource->bytes();
     return make_scoped_ptr(resource);
   }
 
   // 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
   // truly safe to recycle.
   resource_provider_->EnableReadLockFences(resource->id(), true);
 
   memory_usage_bytes_ += resource->bytes();
   ++resource_count_;
   return make_scoped_ptr(resource);
 }
 
 void ResourcePool::ReleaseResource(
     scoped_ptr<ResourcePool::Resource> resource) {
   if (ResourceUsageTooHigh()) {
     memory_usage_bytes_ -= resource->bytes();
     --resource_count_;
     return;
   }
 
   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(
     size_t max_memory_usage_bytes,
     size_t max_unused_memory_usage_bytes,
     size_t max_resource_count) {
   max_memory_usage_bytes_ = max_memory_usage_bytes;
   max_unused_memory_usage_bytes_ = max_unused_memory_usage_bytes;
   max_resource_count_ = max_resource_count;
 
   ReduceResourceUsage();
 }
 
 void ResourcePool::ReduceResourceUsage() {
-  while (!unused_resources_.empty()) {
+  while (!unused_resources_.empty() && !exported_resources_.empty()) {
     if (!ResourceUsageTooHigh())
       break;
 
     // LRU eviction pattern. Most recently used might be blocked by
     // a read lock fence but it's still better to evict the least
     // recently used as it prevents a resource that is hard to reuse
     // because of unique size from being kept around. Resources that
     // 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_;
     delete resource;
   }
 }
 
 bool ResourcePool::ResourceUsageTooHigh() {
   if (resource_count_ > max_resource_count_)
     return true;
   if (memory_usage_bytes_ > max_memory_usage_bytes_)
     return true;
   if (unused_memory_usage_bytes_ > max_unused_memory_usage_bytes_)
     return true;
   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