Idle cleanup for worker context

cc/output/context_cache_controller.cc

Index: cc/output/context_cache_controller.cc
diff --git a/cc/output/context_cache_controller.cc b/cc/output/context_cache_controller.cc
index 218059dd1780531bd90debf3b0f8884b5a92cb6a..d78f4a9ac31ff1241abb3eaa46a1bbd0fcb5dcc2 100644
--- a/cc/output/context_cache_controller.cc
+++ b/cc/output/context_cache_controller.cc
@@ -4,20 +4,26 @@
 
 #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 {
-ContextCacheController::ScopedVisibility::ScopedVisibility() = default;
+namespace {
+static const int kIdleCleanupDelaySeconds = 1;
+}  // namespace
 
-ContextCacheController::ScopedVisibility::~ScopedVisibility() {
+ContextCacheController::ScopedToken::ScopedToken() = default;
+
+ContextCacheController::ScopedToken::~ScopedToken() {
   DCHECK(released_);
 }
 
-void ContextCacheController::ScopedVisibility::Release() {
+void ContextCacheController::ScopedToken::Release() {
   DCHECK(!released_);
   released_ = true;
 }
@@ -25,7 +31,9 @@ void ContextCacheController::ScopedVisibility::Release() {
 ContextCacheController::ContextCacheController(
     gpu::ContextSupport* context_support,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-    : context_support_(context_support), task_runner_(std::move(task_runner)) {}
+    : context_support_(context_support),
+      task_runner_(std::move(task_runner)),
+      weak_factory_(this) {}

[jbauman, 2016/09/22 02:13:43]

You need to do weak_factory_.GetWeakPtr() in the constructor to ensure it's
bound to the right thread so you don't hit DCHECKs.

[ericrk, 2016/09/22 18:35:39]

Hmm... Good point - If we're sure we always use this on the same thread, it
shouldn't matter, but it can't hurt as a sanity check.

[ericrk, 2016/09/22 18:40:51]

Ah, but we don't... yeah... I see what you mean.

 
 ContextCacheController::~ContextCacheController() = default;
 
@@ -33,6 +41,10 @@ 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() {
   bool became_visible = num_clients_visible_ == 0;
@@ -53,20 +65,57 @@ void ContextCacheController::ClientBecameNotVisible(
   --num_clients_visible_;
 
   if (num_clients_visible_ == 0) {
+    // We are freeing resources now - cancel any pending idle callbacks.
+    ++current_idle_generation_;
+
     if (gr_context_)
       gr_context_->freeGpuResources();
     context_support_->SetAggressivelyFreeResources(true);
   }
 }
 
-std::unique_ptr<ContextCacheController::ScopedVisibility>
-ContextCacheController::CreateScopedVisibilityForTesting() const {
-  return base::WrapUnique(new ScopedVisibility());
+std::unique_ptr<ContextCacheController::ScopedBusy>
+ContextCacheController::ClientBecameBusy() {
+  ++num_clients_busy_;
+  // We are busy, cancel any pending idle callbacks.
+  ++current_idle_generation_;
+
+  return base::WrapUnique(new ScopedBusy());
 }
 
-void ContextCacheController::ReleaseScopedVisibilityForTesting(
-    std::unique_ptr<ScopedVisibility> scoped_visibility) const {
-  scoped_visibility->Release();
+void ContextCacheController::ClientBecameNotBusy(
+    std::unique_ptr<ScopedBusy> scoped_busy) {
+  DCHECK(scoped_busy);
+  scoped_busy->Release();
+
+  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_) {
+    task_runner_->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&ContextCacheController::OnIdle, weak_factory_.GetWeakPtr(),
+                   current_idle_generation_),
+        base::TimeDelta::FromSeconds(kIdleCleanupDelaySeconds));

[jbauman, 2016/09/22 02:13:42]

I think that when the context has a lock, that will mean that num_clients_busy
always oscillates between 0 and 1. That means a lot of unnecessary tasks will be
posted.

It would probably be better to either have some timer slack (and not post a new
task if one was posted in the last 0.5 seconds or something) or do something
like base::Timer and have the last posted task post a new task if
current_idle_generation_ was changed since it was posted.

[ericrk, 2016/09/22 18:35:39]

I like your second option I think - we don't need this to run fast, and this
results in the minimal amount of unnecessary tasks.

+  }
+}
+
+void ContextCacheController::OnIdle(uint32_t idle_generation) {
+  std::unique_ptr<base::AutoLock> hold;

[jbauman, 2016/09/22 02:13:43]

This will unnecessarily grab the lock (and block the main thread) if work's
currently happening. Maybe there should be a different lock just for
current_idle_generation_. Or we could try using base::subtle::Atomic, though
that's probably overkill.

[ericrk, 2016/09/22 18:35:39]

Good point - actually meant to do something different here but forgot to get to
it. I've re-worked a bit.

+  if (context_lock_)
+    hold.reset(new base::AutoLock(*context_lock_));
+
+  if (current_idle_generation_ != idle_generation)
+    return;
+
+  if (gr_context_)
+    gr_context_->freeGpuResources();
+
+  // Toggle SetAggressivelyFreeResources to drop command buffer data.
+  context_support_->SetAggressivelyFreeResources(true);
+  context_support_->SetAggressivelyFreeResources(false);
 }
 
 }  // namespace cc