Expire resources in ResourcePool after non-use

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 <algorithm>
 
 #include "base/format_macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "cc/resources/resource_provider.h"
 #include "cc/resources/resource_util.h"
 #include "cc/resources/scoped_resource.h"
 
 namespace cc {
 
+namespace {

[reveman, 2015/08/20 21:43:37]

nit: move this blank line below l.19 instead

[ericrk, 2015/08/21 12:48:22]

I *think* I did what you meant? Tried to match other files.

+// Delay before a resource is considered expired.
+const int kResourceExpirationDelayMs = 1000;
+}  // namespace

[reveman, 2015/08/20 21:43:37]

nit: blank line above this

[ericrk, 2015/08/21 12:48:22]

Done.

+
 void ResourcePool::PoolResource::OnMemoryDump(
     base::trace_event::ProcessMemoryDump* pmd,
     const ResourceProvider* resource_provider,
     bool is_free) const {
   // Resource IDs are not process-unique, so log with the ResourceProvider's
   // unique id.
   std::string parent_node =
       base::StringPrintf("cc/resource_memory/resource_provider_%d/resource_%d",
                          resource_provider->tracing_id(), id());
 
@@ skipping 10 matching lines @@
   dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
                   base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                   total_bytes);
 
   if (is_free) {
     dump->AddScalar("free_size",
                     base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                     total_bytes);
   }
 }
+
 ResourcePool::ResourcePool(ResourceProvider* resource_provider)
-    : resource_provider_(resource_provider),
-      target_(0),
-      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) {
-  base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
-      this, base::ThreadTaskRunnerHandle::Get());
-}
+    : ResourcePool(resource_provider, 0) {}
 
 ResourcePool::ResourcePool(ResourceProvider* resource_provider, GLenum target)
     : resource_provider_(resource_provider),
       target_(target),
       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) {
-  DCHECK_NE(0u, target);
+      resource_count_(0),
+      evict_expired_resources_pending_(false),
+      resource_expiration_delay_(
+          base::TimeDelta::FromMilliseconds(kResourceExpirationDelayMs)),
+      weak_ptr_factory_(this) {
+  evict_expired_resources_callback_ = base::Bind(

[reveman, 2015/08/20 21:43:37]

Unlike one-copy where worker threads are in play, you can just inline  the bind
with the post task instead of doing it here.

[ericrk, 2015/08/21 12:48:22]

ok, makes sense.

+      &ResourcePool::EvictExpiredResources, weak_ptr_factory_.GetWeakPtr());
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       this, base::ThreadTaskRunnerHandle::Get());
 }
 
 ResourcePool::~ResourcePool() {
   base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
       this);
 
   DCHECK_EQ(0u, in_use_resources_.size());
 
@@ skipping 65 matching lines @@
       resource->size(), resource->format());
   return resource;
 }
 
 void ResourcePool::ReleaseResource(Resource* resource, uint64_t content_id) {
   auto it = in_use_resources_.find(resource->id());
   DCHECK(it != in_use_resources_.end());
 
   PoolResource* pool_resource = it->second;
   pool_resource->set_content_id(content_id);
+  pool_resource->set_last_usage(base::TimeTicks::Now());

[reveman, 2015/08/20 21:43:37]

Can we avoid calling Now() two times in this case. Once here and once in
scheduleevict..

Maybe take advantage of the fact that you know that we have evictable resources
here and if no eviction task is currently scheduled then this new unused task is
the one we should schedule an eviction task for.

[ericrk, 2015/08/21 12:48:22]

Ok, refactored so we now only call Now() once in this case and in the
re-scheduling case below.

 
   // Transfer resource to |busy_resources_|.
   busy_resources_.push_back(in_use_resources_.take_and_erase(it));
+
+  // Now that we have evictable resources, schedule cleanup if necessary.
+  ScheduleEvictExpiredResources();
 }
 
 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()) {
     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.
-    scoped_ptr<PoolResource> resource = unused_resources_.take_front();
-    unused_memory_usage_bytes_ -= ResourceUtil::UncheckedSizeInBytes<size_t>(
-        resource->size(), resource->format());
-    DeleteResource(resource.Pass());
+    DeleteUnusedResource(unused_resources_.take_front());
   }
 }
 
 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::DeleteResource(scoped_ptr<PoolResource> resource) {
   size_t resource_bytes = ResourceUtil::UncheckedSizeInBytes<size_t>(
       resource->size(), resource->format());
   memory_usage_bytes_ -= resource_bytes;
   --resource_count_;
 }
 
+void ResourcePool::DeleteUnusedResource(scoped_ptr<PoolResource> resource) {
+  unused_memory_usage_bytes_ -= ResourceUtil::UncheckedSizeInBytes<size_t>(
+      resource->size(), resource->format());
+  DeleteResource(resource.Pass());
+}
+
 void ResourcePool::CheckBusyResources() {
   for (size_t i = 0; i < busy_resources_.size();) {
     ResourceDeque::iterator it(busy_resources_.begin() + i);
     PoolResource* resource = *it;
 
     if (resource_provider_->CanLockForWrite(resource->id())) {
       DidFinishUsingResource(busy_resources_.take(it));
     } else if (resource_provider_->IsLost(resource->id())) {
       // Remove lost resources from pool.
       DeleteResource(busy_resources_.take(it));
     } else {
       ++i;
     }
   }
 }
 
 void ResourcePool::DidFinishUsingResource(scoped_ptr<PoolResource> resource) {
   unused_memory_usage_bytes_ += ResourceUtil::UncheckedSizeInBytes<size_t>(
       resource->size(), resource->format());
   unused_resources_.push_back(resource.Pass());
 }
 
+void ResourcePool::ScheduleEvictExpiredResources() {
+  if (evict_expired_resources_pending_)
+    return;
+
+  if (!HasEvictableResources())
+    return;
+
+  evict_expired_resources_pending_ = true;
+
+  // Schedule a call to EvictExpiredResources at the time when the LRU buffer
+  // expires.
+  base::TimeTicks reduce_resource_usage_time =
+      GetUsageTimeForLRUResource() + resource_expiration_delay_;
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

[reveman, 2015/08/20 21:43:37]

Should this task runner be passed to the ctor instead to be more consistent with
the rest of tile management code?

[ericrk, 2015/08/21 12:48:22]

Sure, we can do that.

+      FROM_HERE, evict_expired_resources_callback_,
+      reduce_resource_usage_time - base::TimeTicks::Now());
+}
+
+void ResourcePool::EvictExpiredResources() {
+  evict_expired_resources_pending_ = false;
+
+  if (!HasEvictableResources())
+    return;
+
+  EvictResourcesNotUsedSince(base::TimeTicks::Now() -
+                             resource_expiration_delay_);
+
+  // Re-schedule this function if necessary.
+  ScheduleEvictExpiredResources();
+}
+
+void ResourcePool::EvictResourcesNotUsedSince(base::TimeTicks time_limit) {
+  while (!unused_resources_.empty()) {
+    // |unused_resources_| is not strictly ordered with regards to last_usage,
+    // as this may not exactly line up with the time a resource became non-busy.
+    // However, this should be roughly ordered, and will only introduce slight
+    // delays in freeing expired resources.
+    if (unused_resources_.front()->last_usage() > time_limit)
+      return;
+
+    DeleteUnusedResource(unused_resources_.take_front());
+  }
+
+  // Also free busy resources older than the delay. With a sufficiently large
+  // delay, such as the 1 second used here, any "busy" resources which have
+  // expired are not likely to be busy. Additionally, freeing a "busy" resource
+  // has no downside other than incorrect accounting.
+  while (!busy_resources_.empty()) {
+    if (busy_resources_.front()->last_usage() > time_limit)
+      return;
+
+    DeleteResource(busy_resources_.take_front());
+  }
+}
+
+bool ResourcePool::HasEvictableResources() const {
+  return !unused_resources_.empty() || !busy_resources_.empty();
+}
+
+base::TimeTicks ResourcePool::GetUsageTimeForLRUResource() const {
+  if (!unused_resources_.empty()) {
+    return unused_resources_.front()->last_usage();
+  }
+
+  if (!busy_resources_.empty()) {

[reveman, 2015/08/20 21:43:37]

DCHECK(!busy_resources_.empty()) instead as we shouldn't call this unless we
have evictable resources.

[ericrk, 2015/08/21 12:48:22]

sounds good

+    return busy_resources_.front()->last_usage();
+  }
+
+  return base::TimeTicks();
+}
+
 bool ResourcePool::OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                                 base::trace_event::ProcessMemoryDump* pmd) {
   for (const auto& resource : unused_resources_) {
     resource->OnMemoryDump(pmd, resource_provider_, true /* is_free */);
   }
   for (const auto& resource : busy_resources_) {
     resource->OnMemoryDump(pmd, resource_provider_, false /* is_free */);
   }
   for (const auto& entry : in_use_resources_) {
     entry.second->OnMemoryDump(pmd, resource_provider_, false /* is_free */);
   }
   return true;
 }
 
 }  // namespace cc