Don't purge busy resources from ResourcePool

But drop them when they become available if we're in the SUSPENDED
memory state.

Deleting busy resorces doesn't actually free any memory, it just moves
the ownership solely into the ResourceProvider until they become
available. Instead, we can keep them in the busy list, and when
they are available, decide to keep them or not based on the
memory state instead.

This should also improve things by reducing memory footprint in the
SUSPENDED memory state if OnPurgeMemory() isn't being called but
new tile resources are being made and released.

R=ericrk@chromium.org, vmpstr@chromium.org

Bug: 730660
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I24e1a6f38c88fa278566849174a315bff71ed375
Reviewed-on: https://chromium-review.googlesource.com/886862
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532492}

cc/resources/resource_pool.cc

@@ -410,7 +410,10 @@ void ResourcePool::CheckBusyResources() {
     PoolResource* resource = it->get();
 
     if (resource_provider_->CanLockForWrite(resource->resource_id())) {
-      DidFinishUsingResource(std::move(*it));
+      if (evict_busy_resources_when_unused_)
+        DeleteResource(std::move(*it));
+      else
+        DidFinishUsingResource(std::move(*it));
       it = busy_resources_.erase(it);
     } else if (resource_provider_->IsLost(resource->resource_id())) {
       // Remove lost resources from pool.
@@ -447,7 +450,7 @@ void ResourcePool::EvictExpiredResources() {
 
   EvictResourcesNotUsedSince(current_time - resource_expiration_delay_);
 
-  if (unused_resources_.empty() && busy_resources_.empty()) {
+  if (unused_resources_.empty()) {
     // If nothing is evictable, we have deleted one (and possibly more)
     // resources without any new activity. Flush to ensure these deletions are
     // processed.
@@ -472,17 +475,6 @@ void ResourcePool::EvictResourcesNotUsedSince(base::TimeTicks time_limit) {
 
     DeleteResource(PopBack(&unused_resources_));
   }
-
-  // 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_.back()->last_usage() > time_limit)
-      return;
-
-    DeleteResource(PopBack(&busy_resources_));
-  }
 }
 
 base::TimeTicks ResourcePool::GetUsageTimeForLRUResource() const {
@@ -523,6 +515,12 @@ void ResourcePool::OnPurgeMemory() {
   EvictResourcesNotUsedSince(base::TimeTicks() + base::TimeDelta::Max());
 }
 
+void ResourcePool::OnMemoryStateChange(base::MemoryState state) {
+  // While in a SUSPENDED state, we don't put resources back into the pool
+  // when they become available. Instead we free them immediately.
+  evict_busy_resources_when_unused_ = state == base::MemoryState::SUSPENDED;
+}
+
 ResourcePool::PoolResource::PoolResource(size_t unique_id,
                                          const gfx::Size& size,
                                          viz::ResourceFormat format,

cc/resources/resource_pool.h

@@ -155,6 +155,7 @@ class CC_EXPORT ResourcePool : public base::trace_event::MemoryDumpProvider,
 
   // Overriden from base::MemoryCoordinatorClient.
   void OnPurgeMemory() override;
+  void OnMemoryStateChange(base::MemoryState state) override;
 
   size_t GetTotalMemoryUsageForTesting() const {
     return total_memory_usage_bytes_;
@@ -257,6 +258,7 @@ class CC_EXPORT ResourcePool : public base::trace_event::MemoryDumpProvider,
   size_t total_memory_usage_bytes_ = 0;
   size_t total_resource_count_ = 0;
   bool evict_expired_resources_pending_ = false;
+  bool evict_busy_resources_when_unused_ = false;
 
   // Holds most recently used resources at the front of the queue.
   base::circular_deque<std::unique_ptr<PoolResource>> unused_resources_;

cc/resources/resource_pool_unittest.cc

@@ -156,7 +156,7 @@ TEST_F(ResourcePoolTest, LostResource) {
   EXPECT_EQ(0u, resource_provider_->num_resources());
 }
 
-TEST_F(ResourcePoolTest, BusyResourcesEventuallyFreed) {
+TEST_F(ResourcePoolTest, BusyResourcesNotFreed) {
   // Set a quick resource expiration delay so that this test doesn't take long
   // to run.
   resource_pool_ = std::make_unique<ResourcePool>(
@@ -192,9 +192,12 @@ TEST_F(ResourcePoolTest, BusyResourcesEventuallyFreed) {
                                 base::TimeDelta::FromMillisecondsD(200));
   run_loop.Run();
 
-  EXPECT_EQ(0u, resource_provider_->num_resources());
-  EXPECT_EQ(0u, resource_pool_->GetTotalMemoryUsageForTesting());
+  // Busy resources are still help, since they may be in flight to the display
+  // compositor and should not be freed.
+  EXPECT_EQ(1u, resource_provider_->num_resources());
+  EXPECT_EQ(40000u, resource_pool_->GetTotalMemoryUsageForTesting());
   EXPECT_EQ(0u, resource_pool_->memory_usage_bytes());
+  EXPECT_EQ(1u, resource_pool_->GetBusyResourceCountForTesting());
 }
 
 TEST_F(ResourcePoolTest, UnusedResourcesEventuallyFreed) {
@@ -382,6 +385,53 @@ TEST_F(ResourcePoolTest, ReuseResource) {
   CheckAndReturnResource(std::move(resource));
 }
 
+TEST_F(ResourcePoolTest, PurgedMemory) {
+  // Limits high enough to not be hit by this test.
+  size_t bytes_limit = 10 * 1024 * 1024;
+  size_t count_limit = 100;
+  resource_pool_->SetResourceUsageLimits(bytes_limit, count_limit);
+
+  gfx::Size size(100, 100);
+  viz::ResourceFormat format = viz::RGBA_8888;
+  gfx::ColorSpace color_space = gfx::ColorSpace::CreateSRGB();
+  ResourcePool::InUsePoolResource resource =
+      resource_pool_->AcquireResource(size, format, color_space);
+
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+  EXPECT_EQ(0u, resource_pool_->GetBusyResourceCountForTesting());
+
+  // Purging and suspending should not impact an in-use resource.
+  resource_pool_->OnPurgeMemory();
+  resource_pool_->OnMemoryStateChange(base::MemoryState::SUSPENDED);
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+  EXPECT_EQ(0u, resource_pool_->GetBusyResourceCountForTesting());
+  resource_pool_->OnMemoryStateChange(base::MemoryState::NORMAL);
+
+  // Release the resource making it busy.
+  resource_pool_->OnMemoryStateChange(base::MemoryState::NORMAL);
+  resource_pool_->ReleaseResource(std::move(resource));
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+  EXPECT_EQ(1u, resource_pool_->GetBusyResourceCountForTesting());
+
+  // Purging and suspending should not impact a busy resource either.
+  resource_pool_->OnPurgeMemory();
+  resource_pool_->OnMemoryStateChange(base::MemoryState::SUSPENDED);
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+  EXPECT_EQ(1u, resource_pool_->GetBusyResourceCountForTesting());
+
+  // The resource moves from busy to available.
+  resource_pool_->OnMemoryStateChange(base::MemoryState::NORMAL);
+  resource_pool_->CheckBusyResources();
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+  EXPECT_EQ(0u, resource_pool_->GetBusyResourceCountForTesting());
+
+  // Purging and suspending should drop unused resources.
+  resource_pool_->OnPurgeMemory();
+  resource_pool_->OnMemoryStateChange(base::MemoryState::SUSPENDED);
+  EXPECT_EQ(0u, resource_pool_->GetTotalResourceCountForTesting());
+  EXPECT_EQ(0u, resource_pool_->GetBusyResourceCountForTesting());
+}
+
 TEST_F(ResourcePoolTest, MemoryStateSuspended) {
   // Limits high enough to not be hit by this test.
   size_t bytes_limit = 10 * 1024 * 1024;
@@ -405,13 +455,20 @@ TEST_F(ResourcePoolTest, MemoryStateSuspended) {
   resource_pool_->OnMemoryStateChange(base::MemoryState::NORMAL);
 
   // Release the resource making it busy.
+  resource_pool_->OnMemoryStateChange(base::MemoryState::NORMAL);
   resource_pool_->ReleaseResource(std::move(resource));
   EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
   EXPECT_EQ(1u, resource_pool_->GetBusyResourceCountForTesting());
 
-  // Purging and suspending should now free the busy resource.
+  // Purging and suspending should not impact a busy resource either.
   resource_pool_->OnPurgeMemory();
   resource_pool_->OnMemoryStateChange(base::MemoryState::SUSPENDED);
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+  EXPECT_EQ(1u, resource_pool_->GetBusyResourceCountForTesting());
+
+  // The resource moves from busy to available, but since we are SUSPENDED
+  // it is not kept.
+  resource_pool_->CheckBusyResources();
   EXPECT_EQ(0u, resource_pool_->GetTotalResourceCountForTesting());
   EXPECT_EQ(0u, resource_pool_->GetBusyResourceCountForTesting());
 }