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 30 matching lines @@
 ResourcePool::~ResourcePool() {
   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()))

[reveman, 2013/10/30 14:56:02]

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.

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

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.

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

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.

[piman, 2013/10/30 21:41:12]

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.

       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);
 
   // 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;

[reveman, 2013/10/30 14:56:02]

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?

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

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.

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

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

[jadahl, 2013/10/31 08:38:42]

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?
Agreed.

[danakj, 2013/10/31 17:27:52]

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.

[reveman, 2013/10/31 19:41:49]

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.

   }
 
   unused_memory_usage_bytes_ += resource->bytes();

[reveman, 2013/10/31 19:41:49]

This needs to be moved to CheckConsumedResources() instead.

-  unused_resources_.push_back(resource.release());
+
+  if (resource_provider_->InUseByConsumer(resource->id()))

[reveman, 2013/10/30 14:56:02]

Can we remove this check and always add released resources to
|consumed_resources_|? I'd prefer to only check InUseByConsumer in one place.

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

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

[reveman, 2013/10/30 18:50:47]

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.

[jadahl, 2013/10/31 08:38:42]

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

[reveman, 2013/10/31 19:41:49]

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

+    consumed_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;
 
@@ skipping 24 matching lines @@
 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::CheckConsumedResources() {
+  ResourceList::iterator it = consumed_resources_.begin();
+  while (it != consumed_resources_.end()) {

[reveman, 2013/10/30 14:56:02]

nit: consider using a temporary "Resource* resource = *it;" here. that's more
consistent with other loops in this file. 

+    if (!resource_provider_->InUseByConsumer((*it)->id())) {
+      unused_resources_.push_back(*it);
+      it = consumed_resources_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+}
+
 }  // namespace cc