Reschedule EvictExpiredResources

crrev.com/c/1284435 introduced a bug where if we flushed due to deadline
but had potentially evictable resources for the future, we would not
ScheduleEvictExpiredResourcesIn

Also added a micro-optimization where if we evict all resources
OnMemoryPressure, we also flush to reclaim it immediately.

Added unittests to cover flushing logic. Dependency injected fake
clock to make test robust.

Bug: 899002
Change-Id: I50ca934170ebe76e7f48326e53e60df14c32c078
Reviewed-on: https://chromium-review.googlesource.com/c/1299601
Commit-Queue: Jonathan Backer <backer@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603921}

cc/resources/resource_pool.cc

@@ -16,6 +16,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/default_tick_clock.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "build/build_config.h"
 #include "cc/base/container_util.h"
@@ -97,6 +98,7 @@ ResourcePool::ResourcePool(
       disallow_non_exact_reuse_(disallow_non_exact_reuse),
       tracing_id_(g_next_tracing_id.GetNext()),
       flush_evicted_resources_deadline_(base::TimeTicks::Max()),
+      clock_(base::DefaultTickClock::GetInstance()),
       weak_ptr_factory_(this) {
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       this, "cc::ResourcePool", task_runner_.get());
@@ -388,7 +390,7 @@ void ResourcePool::ReleaseResource(InUsePoolResource in_use_resource) {
   // crbug.com/598286.
   CHECK(it->second.get());
 
-  pool_resource->set_last_usage(base::TimeTicks::Now());
+  pool_resource->set_last_usage(clock_->NowTicks());
   in_use_memory_usage_bytes_ -=
       viz::ResourceSizes::UncheckedSizeInBytes<size_t>(pool_resource->size(),
                                                        pool_resource->format());
@@ -468,7 +470,7 @@ void ResourcePool::DeleteResource(std::unique_ptr<PoolResource> resource) {
   --total_resource_count_;
   if (flush_evicted_resources_deadline_ == base::TimeTicks::Max()) {
     flush_evicted_resources_deadline_ =
-        base::TimeTicks::Now() + kDefaultMaxFlushDelay;
+        clock_->NowTicks() + kDefaultMaxFlushDelay;
   }
 }
 
@@ -505,31 +507,27 @@ void ResourcePool::ScheduleEvictExpiredResourcesIn(
 
 void ResourcePool::EvictExpiredResources() {
   evict_expired_resources_pending_ = false;
-  base::TimeTicks current_time = base::TimeTicks::Now();
+  base::TimeTicks current_time = clock_->NowTicks();
 
   EvictResourcesNotUsedSince(current_time - resource_expiration_delay_);
 
   if (unused_resources_.empty() ||
-      flush_evicted_resources_deadline_ < current_time) {
-    flush_evicted_resources_deadline_ = base::TimeTicks::Max();
+      flush_evicted_resources_deadline_ <= current_time) {
     // If nothing is evictable, we have deleted one (and possibly more)
     // resources without any new activity. Flush to ensure these deletions are
     // processed.
-    if (context_provider_) {
-      // Flush any ContextGL work as well as any SharedImageInterface work.
-      context_provider_->ContextGL()->OrderingBarrierCHROMIUM();
-      context_provider_->ContextSupport()->FlushPendingWork();
-    }
-    return;
+    FlushEvictedResources();
   }
 
-  // If we still have evictable resources, schedule a call to
-  // EvictExpiredResources for either (a) the time when the LRU buffer expires
-  // or (b) the deadline to explicitly flush previously evicted resources.
-  ScheduleEvictExpiredResourcesIn(
-      std::min(GetUsageTimeForLRUResource() + resource_expiration_delay_,
-               flush_evicted_resources_deadline_) -
-      current_time);
+  if (!unused_resources_.empty()) {
+    // If we still have evictable resources, schedule a call to
+    // EvictExpiredResources for either (a) the time when the LRU buffer expires
+    // or (b) the deadline to explicitly flush previously evicted resources.
+    ScheduleEvictExpiredResourcesIn(
+        std::min(GetUsageTimeForLRUResource() + resource_expiration_delay_,
+                 flush_evicted_resources_deadline_) -
+        current_time);
+  }
 }
 
 void ResourcePool::EvictResourcesNotUsedSince(base::TimeTicks time_limit) {
@@ -555,6 +553,15 @@ base::TimeTicks ResourcePool::GetUsageTimeForLRUResource() const {
   return busy_resources_.back()->last_usage();
 }
 
+void ResourcePool::FlushEvictedResources() {
+  flush_evicted_resources_deadline_ = base::TimeTicks::Max();
+  if (context_provider_) {
+    // Flush any ContextGL work as well as any SharedImageInterface work.
+    context_provider_->ContextGL()->OrderingBarrierCHROMIUM();
+    context_provider_->ContextSupport()->FlushPendingWork();
+  }
+}
+
 bool ResourcePool::OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                                 base::trace_event::ProcessMemoryDump* pmd) {
   if (args.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND) {
@@ -589,6 +596,7 @@ void ResourcePool::OnMemoryPressure(
       break;
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL:
       EvictResourcesNotUsedSince(base::TimeTicks() + base::TimeDelta::Max());
+      FlushEvictedResources();
       break;
   }
 }

cc/resources/resource_pool.h

@@ -16,6 +16,7 @@
 #include "base/memory/memory_pressure_listener.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/time/tick_clock.h"
 #include "base/trace_event/memory_allocator_dump_guid.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/unguessable_token.h"
@@ -264,6 +265,9 @@ class CC_EXPORT ResourcePool : public base::trace_event::MemoryDumpProvider {
     return !disallow_non_exact_reuse_;
   }
 
+  // Overrides internal clock for testing purposes.
+  void SetClockForTesting(const base::TickClock* clock) { clock_ = clock; }
+
  private:
   FRIEND_TEST_ALL_PREFIXES(ResourcePoolTest, ReuseResource);
   FRIEND_TEST_ALL_PREFIXES(ResourcePoolTest, ExactRequestsRespected);
@@ -371,6 +375,7 @@ class CC_EXPORT ResourcePool : public base::trace_event::MemoryDumpProvider {
   void EvictResourcesNotUsedSince(base::TimeTicks time_limit);
   bool HasEvictableResources() const;
   base::TimeTicks GetUsageTimeForLRUResource() const;
+  void FlushEvictedResources();
 
   viz::ClientResourceProvider* const resource_provider_;
   viz::ContextProvider* const context_provider_;
@@ -399,6 +404,8 @@ class CC_EXPORT ResourcePool : public base::trace_event::MemoryDumpProvider {
 
   base::TimeTicks flush_evicted_resources_deadline_;
 
+  const base::TickClock* clock_;
+
   base::WeakPtrFactory<ResourcePool> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePool);

cc/resources/resource_pool_unittest.cc

@@ -6,14 +6,16 @@
 
 #include <stddef.h>
 
-#include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/viz/client/client_resource_provider.h"
 #include "components/viz/common/resources/resource_sizes.h"
 #include "components/viz/common/resources/returned_resource.h"
 #include "components/viz/test/test_context_provider.h"
+#include "components/viz/test/test_context_support.h"
 #include "components/viz/test/test_shared_bitmap_manager.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace cc {
@@ -21,13 +23,17 @@ namespace cc {
 class ResourcePoolTest : public testing::Test {
  public:
   void SetUp() override {
-    context_provider_ = viz::TestContextProvider::Create();
+    auto context_support = std::make_unique<MockContextSupport>();
+    context_support_ = context_support.get();
+    context_provider_ =
+        viz::TestContextProvider::Create(std::move(context_support));
     context_provider_->BindToCurrentThread();
     resource_provider_ = std::make_unique<viz::ClientResourceProvider>(true);
-    task_runner_ = base::ThreadTaskRunnerHandle::Get();
+    test_task_runner_ = base::MakeRefCounted<base::TestMockTimeTaskRunner>();
     resource_pool_ = std::make_unique<ResourcePool>(
-        resource_provider_.get(), context_provider_.get(), task_runner_,
+        resource_provider_.get(), context_provider_.get(), test_task_runner_,
         ResourcePool::kDefaultExpirationDelay, false);
+    resource_pool_->SetClockForTesting(test_task_runner_->GetMockTickClock());
   }
 
   void TearDown() override {
@@ -35,6 +41,12 @@ class ResourcePoolTest : public testing::Test {
   }
 
  protected:
+  class MockContextSupport : public viz::TestContextSupport {
+   public:
+    MockContextSupport() = default;
+    MOCK_METHOD0(FlushPendingWork, void());
+  };
+
   class StubGpuBacking : public ResourcePool::GpuBacking {
    public:
     void OnMemoryDump(
@@ -58,9 +70,10 @@ class ResourcePoolTest : public testing::Test {
   }
 
   viz::TestSharedBitmapManager shared_bitmap_manager_;
+  MockContextSupport* context_support_;
   scoped_refptr<viz::TestContextProvider> context_provider_;
   std::unique_ptr<viz::ClientResourceProvider> resource_provider_;
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  scoped_refptr<base::TestMockTimeTaskRunner> test_task_runner_;
   std::unique_ptr<ResourcePool> resource_pool_;
 };
 
@@ -77,6 +90,67 @@ TEST_F(ResourcePoolTest, AcquireRelease) {
   resource_pool_->ReleaseResource(std::move(resource));
 }
 
+TEST_F(ResourcePoolTest, EventuallyEvictAndFlush) {
+  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);
+  resource_pool_->ReleaseResource(std::move(resource));
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+
+  // Expect flush after eviction and flush delay.
+  EXPECT_CALL(*context_support_, FlushPendingWork()).Times(testing::AtLeast(1));
+  test_task_runner_->FastForwardBy(ResourcePool::kDefaultExpirationDelay +
+                                   ResourcePool::kDefaultMaxFlushDelay);
+  EXPECT_EQ(0u, resource_pool_->GetTotalResourceCountForTesting());
+}
+
+TEST_F(ResourcePoolTest, FlushEvenIfMoreUnusedToEvict) {
+  gfx::Size size(100, 100);
+  viz::ResourceFormat format = viz::RGBA_8888;
+  gfx::ColorSpace color_space = gfx::ColorSpace::CreateSRGB();
+  ResourcePool::InUsePoolResource resource1 =
+      resource_pool_->AcquireResource(size, format, color_space);
+  ResourcePool::InUsePoolResource resource2 =
+      resource_pool_->AcquireResource(size, format, color_space);
+
+  // Time 0: No resources evicted yet.
+  EXPECT_EQ(2u, resource_pool_->GetTotalResourceCountForTesting());
+
+  // Space the resource last_usage out so that they don't expire at the same
+  // time. resource1 last used at time 0 (expires kDefaultExpirationDelay) and
+  // resource2 last used at last_usage_gap (expires kDefaultExpireationDelay +
+  // last_usage_gap).
+  const base::TimeDelta last_usage_gap =
+      ResourcePool::kDefaultMaxFlushDelay * 2;
+  resource_pool_->ReleaseResource(std::move(resource1));
+  test_task_runner_->FastForwardBy(last_usage_gap);
+  resource_pool_->ReleaseResource(std::move(resource2));
+
+  // Time |last_usage_gap|: No resources evicted yet.
+  EXPECT_EQ(2u, resource_pool_->GetTotalResourceCountForTesting());
+
+  // Time |kDefaultExpirationDelay|: resource1 evicted, but not resource2 yet.
+  test_task_runner_->FastForwardBy(ResourcePool::kDefaultExpirationDelay -
+                                   last_usage_gap);
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+
+  // Expect at least one flush kDefaultMaxFlushDelay after an eviction.
+  EXPECT_CALL(*context_support_, FlushPendingWork()).Times(testing::AtLeast(1));
+  test_task_runner_->FastForwardBy(ResourcePool::kDefaultMaxFlushDelay);
+
+  // Time |kDefaultExpirationDelay + kDefaultMaxFlushDelay|:
+  // Check that flush was called and resource2 still not evicted.
+  testing::Mock::VerifyAndClearExpectations(context_support_);
+  EXPECT_EQ(1u, resource_pool_->GetTotalResourceCountForTesting());
+
+  // Wait a long time and resource2 should get evicted and flushed.
+  EXPECT_CALL(*context_support_, FlushPendingWork()).Times(testing::AtLeast(1));
+  test_task_runner_->FastForwardBy(ResourcePool::kDefaultExpirationDelay * 100);
+  EXPECT_EQ(0u, resource_pool_->GetTotalResourceCountForTesting());
+}
+
 TEST_F(ResourcePoolTest, AccountingSingleResource) {
   // Limits high enough to not be hit by this test.
   size_t bytes_limit = 10 * 1024 * 1024;
@@ -184,12 +258,6 @@ TEST_F(ResourcePoolTest, LostResource) {
 }
 
 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>(
-      resource_provider_.get(), context_provider_.get(), task_runner_,
-      base::TimeDelta::FromMilliseconds(10), false);
-
   // Limits high enough to not be hit by this test.
   size_t bytes_limit = 10 * 1024 * 1024;
   size_t count_limit = 100;
@@ -216,12 +284,9 @@ TEST_F(ResourcePoolTest, BusyResourcesNotFreed) {
   EXPECT_EQ(0u, resource_pool_->memory_usage_bytes());
   EXPECT_EQ(1u, resource_pool_->GetBusyResourceCountForTesting());
 
-  // Wait for our resource pool to evict resources. We expect resources to be
-  // released within 10 ms, give the thread up to 200.
-  base::RunLoop run_loop;
-  task_runner_->PostDelayedTask(FROM_HERE, run_loop.QuitClosure(),
-                                base::TimeDelta::FromMillisecondsD(200));
-  run_loop.Run();
+  // Wait for our resource pool to evict resources. Wait 10x the expiration
+  // delay.
+  test_task_runner_->FastForwardBy(ResourcePool::kDefaultExpirationDelay * 10);
 
   // Busy resources are still held, since they may be in flight to the display
   // compositor and should not be freed.
@@ -231,12 +296,6 @@ TEST_F(ResourcePoolTest, BusyResourcesNotFreed) {
 }
 
 TEST_F(ResourcePoolTest, UnusedResourcesEventuallyFreed) {
-  // Set a quick resource expiration delay so that this test doesn't take long
-  // to run.
-  resource_pool_ = std::make_unique<ResourcePool>(
-      resource_provider_.get(), context_provider_.get(), task_runner_,
-      base::TimeDelta::FromMilliseconds(100), false);
-
   // Limits high enough to not be hit by this test.
   size_t bytes_limit = 10 * 1024 * 1024;
   size_t count_limit = 100;
@@ -274,12 +333,9 @@ TEST_F(ResourcePoolTest, UnusedResourcesEventuallyFreed) {
   EXPECT_EQ(0u, resource_pool_->resource_count());
   EXPECT_EQ(0u, resource_pool_->GetBusyResourceCountForTesting());
 
-  // Wait for our resource pool to evict resources. We expect resources to be
-  // released within 100 ms, give the thread up to 200.
-  base::RunLoop run_loop;
-  task_runner_->PostDelayedTask(FROM_HERE, run_loop.QuitClosure(),
-                                base::TimeDelta::FromMillisecondsD(200));
-  run_loop.Run();
+  // Wait for our resource pool to evict resources. Wait 10x the expiration
+  // delay.
+  test_task_runner_->FastForwardBy(ResourcePool::kDefaultExpirationDelay * 10);
 
   EXPECT_EQ(0u, resource_pool_->GetTotalMemoryUsageForTesting());
 }
@@ -583,8 +639,8 @@ TEST_F(ResourcePoolTest, ExactRequestsRespected) {
   gfx::ColorSpace color_space = gfx::ColorSpace::CreateSRGB();
 
   resource_pool_ = std::make_unique<ResourcePool>(
-      resource_provider_.get(), context_provider_.get(), task_runner_,
-      base::TimeDelta::FromMilliseconds(100), true);
+      resource_provider_.get(), context_provider_.get(), test_task_runner_,
+      ResourcePool::kDefaultExpirationDelay, true);
 
   // Create unused resource with size 100x100.
   CheckAndReturnResource(resource_pool_->AcquireResource(gfx::Size(100, 100),