Revert of Idle cleanup for worker context (patchset #5 id:200001 of...

Revert of Idle cleanup for worker context (patchset #5 id:200001 of https://codereview.chromium.org/2353033003/ )

Reason for revert:
This appears to be breaking some number of unittests - need to investigate further.

Original issue's description:
> Idle cleanup for worker context
>
> Currently, Skia flushes unused items out of its caches after ~1 second
> of non-use (50 flushes). This works fine while Skia is in-use, but when
> a worker context goes idle we stop calling into Skia altogether. This
> means that Skia will never flush out unused cache items from the last
> piece of work it did. This can lead to some rather large temporaries
> being kept around (~20mb).
>
> This change adds an idle cleanup process which flushes Skia's caches
> and cleans up worker context resources after a worker context is idle
> for 1 second.
>
> BUG=624630
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
>
> Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb
> Cr-Commit-Position: refs/heads/master@{#420496}

TBR=danakj@chromium.org,jbauman@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=624630

Review-Url: https://codereview.chromium.org/2366033002
Cr-Commit-Position: refs/heads/master@{#420741}

cc/BUILD.gn

@@ -216,7 +216,6 @@ component("cc") {
     "output/compositor_frame_sink_client.h",
     "output/context_cache_controller.cc",
     "output/context_cache_controller.h",
-    "output/context_provider.cc",
     "output/context_provider.h",
     "output/copy_output_request.cc",
     "output/copy_output_request.h",

cc/output/context_cache_controller.cc

@@ -4,26 +4,20 @@
 
 #include "cc/output/context_cache_controller.h"
 
-#include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
-#include "base/synchronization/lock.h"
 #include "gpu/command_buffer/client/context_support.h"
 #include "third_party/skia/include/gpu/GrContext.h"
 
 namespace cc {
-namespace {
-static const int kIdleCleanupDelaySeconds = 1;
-}  // namespace
+ContextCacheController::ScopedVisibility::ScopedVisibility() = default;
 
-ContextCacheController::ScopedToken::ScopedToken() = default;
-
-ContextCacheController::ScopedToken::~ScopedToken() {
+ContextCacheController::ScopedVisibility::~ScopedVisibility() {
   DCHECK(released_);
 }
 
-void ContextCacheController::ScopedToken::Release() {
+void ContextCacheController::ScopedVisibility::Release() {
   DCHECK(!released_);
   released_ = true;
 }
@@ -31,16 +25,7 @@ void ContextCacheController::ScopedToken::Release() {
 ContextCacheController::ContextCacheController(
     gpu::ContextSupport* context_support,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-    : context_support_(context_support),
-      task_runner_(std::move(task_runner)),
-      weak_factory_(this) {
-  // The |weak_factory_| can only be used from a single thread. We
-  // create/destroy this class and run callbacks on a single thread, but we
-  // want to be able to post callbacks from multiple threads. We need a weak
-  // ptr to post callbacks, so acquire one here, while we're on the right
-  // thread.
-  weak_ptr_ = weak_factory_.GetWeakPtr();
-}
+    : context_support_(context_support), task_runner_(std::move(task_runner)) {}
 
 ContextCacheController::~ContextCacheController() = default;
 
@@ -48,15 +33,8 @@ void ContextCacheController::SetGrContext(GrContext* gr_context) {
   gr_context_ = gr_context;
 }
 
-void ContextCacheController::SetLock(base::Lock* lock) {
-  context_lock_ = lock;
-}
-
 std::unique_ptr<ContextCacheController::ScopedVisibility>
 ContextCacheController::ClientBecameVisible() {
-  if (context_lock_)
-    context_lock_->AssertAcquired();
-
   bool became_visible = num_clients_visible_ == 0;
   ++num_clients_visible_;
 
@@ -71,105 +49,24 @@ void ContextCacheController::ClientBecameNotVisible(
   DCHECK(scoped_visibility);
   scoped_visibility->Release();
 
-  if (context_lock_)
-    context_lock_->AssertAcquired();
-
   DCHECK_GT(num_clients_visible_, 0u);
   --num_clients_visible_;
 
   if (num_clients_visible_ == 0) {
-    // We are freeing resources now - cancel any pending idle callbacks.
-    InvalidatePendingIdleCallbacks();
-
     if (gr_context_)
       gr_context_->freeGpuResources();
     context_support_->SetAggressivelyFreeResources(true);
   }
 }
 
-std::unique_ptr<ContextCacheController::ScopedBusy>
-ContextCacheController::ClientBecameBusy() {
-  if (context_lock_)
-    context_lock_->AssertAcquired();
-
-  ++num_clients_busy_;
-  // We are busy, cancel any pending idle callbacks.
-  InvalidatePendingIdleCallbacks();
-
-  return base::WrapUnique(new ScopedBusy());
-}
-
-void ContextCacheController::ClientBecameNotBusy(
-    std::unique_ptr<ScopedBusy> scoped_busy) {
-  DCHECK(scoped_busy);
-  scoped_busy->Release();
-
-  if (context_lock_)
-    context_lock_->AssertAcquired();
-
-  DCHECK_GT(num_clients_busy_, 0u);
-  --num_clients_busy_;
-
-  // If we have become idle and we are visible, queue a task to drop resources
-  // after a delay. If are not visible, we have already dropped resources.
-  if (num_clients_busy_ == 0 && num_clients_visible_ > 0 && task_runner_) {
-    // If we already have a callback pending, don't post a new one. The pending
-    // callback will handle posting a new callback itself. This prevents us from
-    // flooding the system with tasks.
-    if (!callback_pending_) {
-      {
-        base::AutoLock hold(current_idle_generation_lock_);
-        PostIdleCallback(current_idle_generation_);
-      }
-      callback_pending_ = true;
-    }
-  }
-}
-
-void ContextCacheController::PostIdleCallback(
-    uint32_t current_idle_generation) const {
-  task_runner_->PostDelayedTask(
-      FROM_HERE, base::Bind(&ContextCacheController::OnIdle, weak_ptr_,
-                            current_idle_generation),
-      base::TimeDelta::FromSeconds(kIdleCleanupDelaySeconds));
-}
-
-void ContextCacheController::InvalidatePendingIdleCallbacks() {
-  base::AutoLock hold(current_idle_generation_lock_);
-  ++current_idle_generation_;
+std::unique_ptr<ContextCacheController::ScopedVisibility>
+ContextCacheController::CreateScopedVisibilityForTesting() const {
+  return base::WrapUnique(new ScopedVisibility());
 }
 
-void ContextCacheController::OnIdle(uint32_t idle_generation) {
-  // First check if we should run our idle callback at all. If we have become
-  // busy since scheduling, just schedule another idle callback and return.
-  {
-    base::AutoLock hold(current_idle_generation_lock_);
-    if (current_idle_generation_ != idle_generation) {
-      PostIdleCallback(current_idle_generation_);
-      return;
-    }
-  }
-
-  // Try to acquire the context lock - if we can't acquire it then we've become
-  // busy since checking |current_idle_generation_| above. In this case, just
-  // re-post our idle callback and return.
-  if (context_lock_ && !context_lock_->Try()) {
-    base::AutoLock hold(current_idle_generation_lock_);
-    PostIdleCallback(current_idle_generation_);
-    return;
-  }
-
-  if (gr_context_)
-    gr_context_->freeGpuResources();
-
-  // Toggle SetAggressivelyFreeResources to drop command buffer data.
-  context_support_->SetAggressivelyFreeResources(true);
-  context_support_->SetAggressivelyFreeResources(false);
-
-  callback_pending_ = false;
-
-  if (context_lock_)
-    context_lock_->Release();
+void ContextCacheController::ReleaseScopedVisibilityForTesting(
+    std::unique_ptr<ScopedVisibility> scoped_visibility) const {
+  scoped_visibility->Release();
 }
 
 }  // namespace cc

cc/output/context_cache_controller.h

@@ -10,13 +10,11 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
 #include "cc/base/cc_export.h"
 
 class GrContext;
 
 namespace base {
-class Lock;
 class SingleThreadTaskRunner;
 }
 
@@ -27,26 +25,26 @@ class ContextSupport;
 namespace cc {
 
 // ContextCacheController manages clearing cached data on ContextProvider when
-// appropriate. Currently, cache clearing is triggered when the Context
-// provider transitions from Visible to Not Visible, or from Busy to Idle. As a
-// ContextProvider may have multiple clients, ContextCacheController tracks
-// visibility and idle status across all clients and only cleans up when
-// appropriate.
+// appropriate. Currently, cache clearing happens when the ContextProvider
+// transitions from visible to not visible. As a ContextProvider may have
+// multiple clients, ContextCacheController tracks visibility across all
+// clients and only cleans up when appropriate.
+//
+// Note: Virtuals on this function are for testing only. This function is not
+// designed to have multiple implementations.
 class CC_EXPORT ContextCacheController {
  public:
-  class CC_EXPORT ScopedToken {
+  class CC_EXPORT ScopedVisibility {
    public:
-    ~ScopedToken();
+    ~ScopedVisibility();
 
    private:
     friend class ContextCacheController;
-    ScopedToken();
+    ScopedVisibility();
     void Release();
 
     bool released_ = false;
   };
-  using ScopedVisibility = ScopedToken;
-  using ScopedBusy = ScopedToken;
 
   ContextCacheController(
       gpu::ContextSupport* context_support,
@@ -54,7 +52,6 @@ class CC_EXPORT ContextCacheController {
   virtual ~ContextCacheController();
 
   void SetGrContext(GrContext* gr_context);
-  void SetLock(base::Lock* lock);
 
   // Clients of the owning ContextProvider should call this function when they
   // become visible. The returned ScopedVisibility pointer must be passed back
@@ -67,42 +64,17 @@ class CC_EXPORT ContextCacheController {
   virtual void ClientBecameNotVisible(
       std::unique_ptr<ScopedVisibility> scoped_visibility);
 
-  // Clients of the owning ContextProvider may call this function when they
-  // become busy. The returned ScopedBusy pointer must be passed back
-  // to ClientBecameNotBusy or it will DCHECK in its destructor.
-  std::unique_ptr<ScopedBusy> ClientBecameBusy();
-
-  // When a client becomes not busy, it must pass back any ScopedBusy
-  // pointers it owns via this function.
-  void ClientBecameNotBusy(std::unique_ptr<ScopedBusy> scoped_busy);
+ protected:
+  std::unique_ptr<ScopedVisibility> CreateScopedVisibilityForTesting() const;
+  void ReleaseScopedVisibilityForTesting(
+      std::unique_ptr<ScopedVisibility> scoped_visibility) const;
 
  private:
-  void OnIdle(uint32_t idle_generation);
-  void PostIdleCallback(uint32_t current_idle_generation) const;
-  void InvalidatePendingIdleCallbacks();
-
   gpu::ContextSupport* context_support_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   GrContext* gr_context_ = nullptr;
 
-  // If set, |context_lock_| must be held before accessing any member within
-  // the idle callback. Exceptions to this are |current_idle_generation_|,
-  // which has its own lock, and weak_ptr_ and task_runner_, which may be
-  // accessed from multiple threads without locking.
-  base::Lock* context_lock_ = nullptr;
-
   uint32_t num_clients_visible_ = 0;
-  uint32_t num_clients_busy_ = 0;
-  bool callback_pending_ = false;
-
-  // |current_idle_generation_lock_| must be held when accessing
-  // |current_idle_generation_|. |current_idle_generation_lock_| must never be
-  // held while acquiring |context_lock_|.
-  base::Lock current_idle_generation_lock_;
-  uint32_t current_idle_generation_ = 0;
-
-  base::WeakPtr<ContextCacheController> weak_ptr_;
-  base::WeakPtrFactory<ContextCacheController> weak_factory_;
 };
 
 }  // namespace cc

cc/output/context_cache_controller_unittest.cc

@@ -5,7 +5,6 @@
 #include "cc/output/context_cache_controller.h"
 
 #include "base/memory/ptr_util.h"
-#include "base/test/test_mock_time_task_runner.h"
 #include "cc/test/test_context_provider.h"
 #include "cc/test/test_context_support.h"
 #include "cc/test/test_web_graphics_context_3d.h"
@@ -26,8 +25,7 @@ class MockContextSupport : public TestContextSupport {
 
 TEST(ContextCacheControllerTest, ScopedVisibilityBasic) {
   StrictMock<MockContextSupport> context_support;
-  auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner);
-  ContextCacheController cache_controller(&context_support, task_runner);
+  ContextCacheController cache_controller(&context_support, nullptr);
 
   EXPECT_CALL(context_support, SetAggressivelyFreeResources(false));
   std::unique_ptr<ContextCacheController::ScopedVisibility> visibility =
@@ -40,76 +38,19 @@ TEST(ContextCacheControllerTest, ScopedVisibilityBasic) {
 
 TEST(ContextCacheControllerTest, ScopedVisibilityMulti) {
   StrictMock<MockContextSupport> context_support;
-  auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner);
-  ContextCacheController cache_controller(&context_support, task_runner);
+  ContextCacheController cache_controller(&context_support, nullptr);
 
   EXPECT_CALL(context_support, SetAggressivelyFreeResources(false));
-  auto visibility_1 = cache_controller.ClientBecameVisible();
+  std::unique_ptr<ContextCacheController::ScopedVisibility> visibility_1 =
+      cache_controller.ClientBecameVisible();
   Mock::VerifyAndClearExpectations(&context_support);
-  auto visibility_2 = cache_controller.ClientBecameVisible();
+  std::unique_ptr<ContextCacheController::ScopedVisibility> visibility_2 =
+      cache_controller.ClientBecameVisible();
 
   cache_controller.ClientBecameNotVisible(std::move(visibility_1));
   EXPECT_CALL(context_support, SetAggressivelyFreeResources(true));
   cache_controller.ClientBecameNotVisible(std::move(visibility_2));
 }
 
-TEST(ContextCacheControllerTest, ScopedBusyWhileVisible) {
-  StrictMock<MockContextSupport> context_support;
-  auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner);
-  ContextCacheController cache_controller(&context_support, task_runner);
-
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(false));
-  auto visibility = cache_controller.ClientBecameVisible();
-  Mock::VerifyAndClearExpectations(&context_support);
-
-  // Now that we're visible, ensure that going idle triggers a delayed cleanup.
-  auto busy = cache_controller.ClientBecameBusy();
-  cache_controller.ClientBecameNotBusy(std::move(busy));
-
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(true));
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(false));
-  task_runner->FastForwardBy(base::TimeDelta::FromSeconds(5));
-  Mock::VerifyAndClearExpectations(&context_support);
-
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(true));
-  cache_controller.ClientBecameNotVisible(std::move(visibility));
-}
-
-TEST(ContextCacheControllerTest, ScopedBusyWhileNotVisible) {
-  StrictMock<MockContextSupport> context_support;
-  auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner);
-  ContextCacheController cache_controller(&context_support, task_runner);
-
-  auto busy = cache_controller.ClientBecameBusy();
-
-  // We are not visible, so becoming busy should not trigger an idle callback.
-  cache_controller.ClientBecameNotBusy(std::move(busy));
-  task_runner->FastForwardBy(base::TimeDelta::FromSeconds(5));
-}
-
-TEST(ContextCacheControllerTest, ScopedBusyMulitpleWhileVisible) {
-  StrictMock<MockContextSupport> context_support;
-  auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner);
-  ContextCacheController cache_controller(&context_support, task_runner);
-
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(false));
-  auto visible = cache_controller.ClientBecameVisible();
-  Mock::VerifyAndClearExpectations(&context_support);
-
-  auto busy_1 = cache_controller.ClientBecameBusy();
-  cache_controller.ClientBecameNotBusy(std::move(busy_1));
-  auto busy_2 = cache_controller.ClientBecameBusy();
-  cache_controller.ClientBecameNotBusy(std::move(busy_2));
-
-  // When we fast forward, only one cleanup should happen.
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(true));
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(false));
-  task_runner->FastForwardBy(base::TimeDelta::FromSeconds(5));
-  Mock::VerifyAndClearExpectations(&context_support);
-
-  EXPECT_CALL(context_support, SetAggressivelyFreeResources(true));
-  cache_controller.ClientBecameNotVisible(std::move(visible));
-}
-
 }  // namespace
 }  // namespace cc

cc/output/context_provider.cc

-// Copyright (c) 2016 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/output/context_provider.h"
-
-namespace cc {
-
-ContextProvider::ScopedContextLock::ScopedContextLock(
-    ContextProvider* context_provider)
-    : context_provider_(context_provider),
-      context_lock_(*context_provider_->GetLock()) {
-  // Allow current thread to use |context_provider_|.
-  context_provider_->DetachFromThread();
-  busy_ = context_provider_->CacheController()->ClientBecameBusy();
-}
-
-ContextProvider::ScopedContextLock::~ScopedContextLock() {
-  // Let ContextCacheController know we are no longer busy.
-  context_provider_->CacheController()->ClientBecameNotBusy(std::move(busy_));
-
-  // Allow usage by thread for which |context_provider_| is bound to.
-  context_provider_->DetachFromThread();
-}
-
-}  // namespace cc

cc/output/context_provider.h

@@ -12,7 +12,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
 #include "cc/base/cc_export.h"
-#include "cc/output/context_cache_controller.h"
 #include "gpu/command_buffer/common/capabilities.h"
 
 class GrContext;
@@ -27,6 +26,7 @@ namespace gles2 { class GLES2Interface; }
 }
 
 namespace cc {
+class ContextCacheController;
 struct ManagedMemoryPolicy;
 
 class ContextProvider : public base::RefCountedThreadSafe<ContextProvider> {
@@ -36,10 +36,18 @@ class ContextProvider : public base::RefCountedThreadSafe<ContextProvider> {
   // lock from GetLock(), so is not always supported. Most use of
   // ContextProvider should be single-thread only on the thread that
   // BindToCurrentThread is run on.
-  class CC_EXPORT ScopedContextLock {
+  class ScopedContextLock {
    public:
-    explicit ScopedContextLock(ContextProvider* context_provider);
-    ~ScopedContextLock();
+    explicit ScopedContextLock(ContextProvider* context_provider)
+        : context_provider_(context_provider),
+          context_lock_(*context_provider_->GetLock()) {
+      // Allow current thread to use |context_provider_|.
+      context_provider_->DetachFromThread();
+    }
+    ~ScopedContextLock() {
+      // Allow usage by thread for which |context_provider_| is bound to.
+      context_provider_->DetachFromThread();
+    }
 
     gpu::gles2::GLES2Interface* ContextGL() {
       return context_provider_->ContextGL();
@@ -48,7 +56,6 @@ class ContextProvider : public base::RefCountedThreadSafe<ContextProvider> {
    private:
     ContextProvider* const context_provider_;
     base::AutoLock context_lock_;
-    std::unique_ptr<ContextCacheController::ScopedBusy> busy_;
   };
 
   // Bind the 3d context to the current thread. This should be called before

content/common/gpu/client/context_provider_command_buffer.cc

@@ -245,13 +245,11 @@ bool ContextProviderCommandBuffer::BindToCurrentThread() {
   ContextGL()->TraceBeginCHROMIUM("gpu_toplevel", unique_context_name.c_str());
   // If support_locking_ is true, the context may be used from multiple
   // threads, and any async callstacks will need to hold the same lock, so
-  // give it to the command buffer and cache controller.
+  // give it to the command buffer.
   // We don't hold a lock here since there's no need, so set the lock very last
   // to prevent asserts that we're not holding it.
-  if (support_locking_) {
+  if (support_locking_)
     command_buffer_->SetLock(&context_lock_);
-    cache_controller_->SetLock(&context_lock_);
-  }
   return true;
 }