cc: Send shared variables between main and impl side using the channel

cc/trees/thread_proxy.cc

Index: cc/trees/thread_proxy.cc
diff --git a/cc/trees/thread_proxy.cc b/cc/trees/thread_proxy.cc
index 86e834023b1a0398f5cd5a41c18001f66afcae23..d80f84092ce4ced89d4e325ed77c0ea354119be8 100644
--- a/cc/trees/thread_proxy.cc
+++ b/cc/trees/thread_proxy.cc
@@ -62,8 +62,7 @@ ThreadProxy::ThreadProxy(
     scoped_refptr<base::SingleThreadTaskRunner> impl_task_runner,
     scoped_ptr<BeginFrameSource> external_begin_frame_source)
     : Proxy(main_task_runner, impl_task_runner),
-      main_thread_only_vars_unsafe_(this, layer_tree_host->id()),
-      main_thread_or_blocked_vars_unsafe_(layer_tree_host),
+      main_thread_only_vars_unsafe_(this, layer_tree_host),
       compositor_thread_vars_unsafe_(
           this,
           layer_tree_host->id(),
@@ -79,11 +78,13 @@ ThreadProxy::ThreadProxy(
 }
 
 ThreadProxy::MainThreadOnly::MainThreadOnly(ThreadProxy* proxy,
-                                            int layer_tree_host_id)
-    : layer_tree_host_id(layer_tree_host_id),
+                                            LayerTreeHost* layer_tree_host)
+    : layer_tree_host_id(layer_tree_host->id()),
+      layer_tree_host(layer_tree_host),
       max_requested_pipeline_stage(NO_PIPELINE_STAGE),
       current_pipeline_stage(NO_PIPELINE_STAGE),
       final_pipeline_stage(NO_PIPELINE_STAGE),
+      commit_waits_for_activation(false),
       started(false),
       prepare_tiles_pending(false),
       defer_commits(false),
@@ -91,13 +92,10 @@ ThreadProxy::MainThreadOnly::MainThreadOnly(ThreadProxy* proxy,
 
 ThreadProxy::MainThreadOnly::~MainThreadOnly() {}
 
-ThreadProxy::MainThreadOrBlockedMainThread::MainThreadOrBlockedMainThread(
-    LayerTreeHost* host)
-    : layer_tree_host(host),
-      commit_waits_for_activation(false),
-      main_thread_inside_commit(false) {}
+ThreadProxy::BlockedMainCommitOnly::BlockedMainCommitOnly()
+    : layer_tree_host(nullptr) {}
 
-ThreadProxy::MainThreadOrBlockedMainThread::~MainThreadOrBlockedMainThread() {}
+ThreadProxy::BlockedMainCommitOnly::~BlockedMainCommitOnly() {}
 
 ThreadProxy::CompositorThreadOnly::CompositorThreadOnly(
     ThreadProxy* proxy,
@@ -105,8 +103,8 @@ ThreadProxy::CompositorThreadOnly::CompositorThreadOnly(
     RenderingStatsInstrumentation* rendering_stats_instrumentation,
     scoped_ptr<BeginFrameSource> external_begin_frame_source)
     : layer_tree_host_id(layer_tree_host_id),
+      next_commit_waits_for_activation(false),
       commit_completion_event(nullptr),
-      completion_event_for_commit_held_on_tree_activation(nullptr),
       next_frame_is_newly_committed_frame(false),
       inside_draw(false),
       input_throttled_until_commit(false),
@@ -408,8 +406,7 @@ void ThreadProxy::SetNeedsRedrawOnImpl(const gfx::Rect& damage_rect) {
 
 void ThreadProxy::SetNextCommitWaitsForActivation() {
   DCHECK(IsMainThread());
-  DCHECK(!blocked_main().main_thread_inside_commit);
-  blocked_main().commit_waits_for_activation = true;
+  main().commit_waits_for_activation = true;
 }
 
 void ThreadProxy::SetDeferCommits(bool defer_commits) {
@@ -491,11 +488,11 @@ void ThreadProxy::SetInputThrottledUntilCommitOnImpl(bool is_throttled) {
 }
 
 LayerTreeHost* ThreadProxy::layer_tree_host() {

[vmpstr, 2015/10/29 21:36:33]

I think we maybe should remove these functions and call main().layer_tree_host
explicitly to make it clear that it can't be accessed on the impl thread.

[Khushal, 2015/10/29 22:52:19]

Done.

-  return blocked_main().layer_tree_host;
+  return main().layer_tree_host;
 }
 
 const LayerTreeHost* ThreadProxy::layer_tree_host() const {
-  return blocked_main().layer_tree_host;
+  return main().layer_tree_host;
 }
 
 ThreadProxy::MainThreadOnly& ThreadProxy::main() {
@@ -507,15 +504,9 @@ const ThreadProxy::MainThreadOnly& ThreadProxy::main() const {
   return main_thread_only_vars_unsafe_;
 }
 
-ThreadProxy::MainThreadOrBlockedMainThread& ThreadProxy::blocked_main() {
-  DCHECK(IsMainThread() || IsMainThreadBlocked());
-  return main_thread_or_blocked_vars_unsafe_;
-}
-
-const ThreadProxy::MainThreadOrBlockedMainThread& ThreadProxy::blocked_main()
-    const {
-  DCHECK(IsMainThread() || IsMainThreadBlocked());
-  return main_thread_or_blocked_vars_unsafe_;
+ThreadProxy::BlockedMainCommitOnly& ThreadProxy::blocked_main_commit() {
+  DCHECK(IsMainThreadBlocked() && impl().commit_completion_event);

[vmpstr, 2015/10/29 21:36:33]

nit: Split into two dchecks, so we know which one fails.

[Khushal, 2015/10/29 22:52:19]

Done.

+  return main_thread_blocked_commit_vars_unsafe_;
 }
 
 ThreadProxy::CompositorThreadOnly& ThreadProxy::impl() {
@@ -535,11 +526,7 @@ void ThreadProxy::Start() {
   // Create LayerTreeHostImpl.
   DebugScopedSetMainThreadBlocked main_thread_blocked(this);
   CompletionEvent completion;
-  Proxy::ImplThreadTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(&ThreadProxy::InitializeImplOnImplThread,
-                 base::Unretained(this),
-                 &completion));
+  main().channel_main->InitializeImplOnImpl(&completion, layer_tree_host());
   completion.Wait();
 
   main_thread_weak_ptr_ = main().weak_factory.GetWeakPtr();
@@ -566,16 +553,12 @@ void ThreadProxy::Stop() {
     DebugScopedSetMainThreadBlocked main_thread_blocked(this);
 
     CompletionEvent completion;
-    Proxy::ImplThreadTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&ThreadProxy::LayerTreeHostClosedOnImplThread,
-                   impl_thread_weak_ptr_,
-                   &completion));
+    main().channel_main->LayerTreeHostClosedOnImpl(&completion);
     completion.Wait();
   }
 
   main().weak_factory.InvalidateWeakPtrs();
-  blocked_main().layer_tree_host = NULL;
+  main().layer_tree_host = nullptr;
   main().started = false;
 }
 
@@ -721,8 +704,11 @@ void ThreadProxy::BeginMainFrame(
     BlockingTaskRunner::CapturePostTasks blocked(
         blocking_main_thread_task_runner());
 
+    bool hold_commit_for_activation = main().commit_waits_for_activation;

[brianderson, 2015/10/29 22:38:28]

I think StartCommitOnImpl will not modify main().commit_waits_for_activation, so
this temp variable isn't necessary and you can reset
main().commit_waits_for_activation at the end of this block.

[Khushal, 2015/10/29 22:52:19]

Done.

+    main().commit_waits_for_activation = false;
     CompletionEvent completion;
-    main().channel_main->StartCommitOnImpl(&completion);
+    main().channel_main->StartCommitOnImpl(&completion, layer_tree_host(),
+                                           hold_commit_for_activation);
     completion.Wait();
   }
 
@@ -737,13 +723,22 @@ void ThreadProxy::BeginMainFrameNotExpectedSoon() {
   layer_tree_host()->BeginMainFrameNotExpectedSoon();
 }
 
-void ThreadProxy::StartCommitOnImpl(CompletionEvent* completion) {
+void ThreadProxy::StartCommitOnImpl(CompletionEvent* completion,
+                                    LayerTreeHost* layer_tree_host,
+                                    bool hold_commit_for_activation) {
   TRACE_EVENT0("cc", "ThreadProxy::StartCommitOnImplThread");
   DCHECK(!impl().commit_completion_event);
   DCHECK(IsImplThread() && IsMainThreadBlocked());
   DCHECK(impl().scheduler);
   DCHECK(impl().scheduler->CommitPending());
 
+  if (hold_commit_for_activation) {

[vmpstr, 2015/10/29 21:36:33]

Can we get multiple StartCommitOnImpl calls while waiting on the same
activation? That is, can we have

impl().next_commit_waits_for_activation be true here and
hold_commit_for_activation be false?

What situations are those?

[Khushal, 2015/10/29 22:52:19]

Once the main side sends the hold_commit_for_activation flag to impl, it will
reset it in main().

So there can be a scenario where the impl().next_commit_waits_for_activation
will get set to true but we early out below. In this case the next time the
start commit call is received, impl().next_commit_waits_for_activation will be
true here but
hold_commit_for_activation will be false since it was cleared in the last call.

+    // This commit may be aborted. Store the value for
+    // hold_commit_for_activation so that whenever the next commit is started,
+    // the main thread will be unblocked only after pending tree activation.
+    impl().next_commit_waits_for_activation = hold_commit_for_activation;
+  }
+
   if (!impl().layer_tree_host_impl) {
     TRACE_EVENT_INSTANT0(
         "cc", "EarlyOut_NoLayerTree", TRACE_EVENT_SCOPE_THREAD);
@@ -755,6 +750,8 @@ void ThreadProxy::StartCommitOnImpl(CompletionEvent* completion) {
   // But, we can avoid a PostTask in here.
   impl().scheduler->NotifyBeginMainFrameStarted();
   impl().commit_completion_event = completion;

[vmpstr, 2015/10/29 21:36:33]

DCHECK that this is currently null? It should be right? Or maybe it can be the
previously held commit completion. I'm not sure overwriting it is a good idea
since then we leak a completion

[Khushal, 2015/10/29 22:52:19]

Its already DCHECKed at the beginning of the function. It should be null since
if it isn't this means that the main thread is blocked right now and we
shouldn't be receiving this call at all.

+  DCHECK(!blocked_main_commit().layer_tree_host);
+  blocked_main_commit().layer_tree_host = layer_tree_host;
   impl().scheduler->NotifyReadyToCommit();
 }
 
@@ -787,24 +784,20 @@ void ThreadProxy::ScheduledActionCommit() {
   DCHECK(IsImplThread());
   DCHECK(IsMainThreadBlocked());
   DCHECK(impl().commit_completion_event);
+  DCHECK(blocked_main_commit().layer_tree_host);
 
-  blocked_main().main_thread_inside_commit = true;
   impl().layer_tree_host_impl->BeginCommit();
-  layer_tree_host()->FinishCommitOnImplThread(
+  blocked_main_commit().layer_tree_host->FinishCommitOnImplThread(
       impl().layer_tree_host_impl.get());
-  blocked_main().main_thread_inside_commit = false;
 
-  bool hold_commit = blocked_main().commit_waits_for_activation;
-  blocked_main().commit_waits_for_activation = false;
+  // Remove the LayerTreeHost reference before the completion event is signaled.

[vmpstr, 2015/10/29 21:36:33]

Why is this important? That is, I understand that we should clear it but the
comment reads that it must happen before the signal

[Khushal, 2015/10/29 22:52:19]

The moment we signal the event, it is cleared as well. And the
blocked_main_commit() accessor DCHECKs the impl().commit_completion_event to
ensure that the LayerTreeHost is accessed only while we are blocked for a
commit. Trying to clear it after clearing the event will fail that check.

+  blocked_main_commit().layer_tree_host = nullptr;
 
-  if (hold_commit) {
+  if (impl().next_commit_waits_for_activation) {
     // For some layer types in impl-side painting, the commit is held until
     // the sync tree is activated.  It's also possible that the
     // sync tree has already activated if there was no work to be done.
     TRACE_EVENT_INSTANT0("cc", "HoldCommit", TRACE_EVENT_SCOPE_THREAD);
-    impl().completion_event_for_commit_held_on_tree_activation =

[vmpstr, 2015/10/29 21:36:33]

This is the part that ensured that we had both completion events if we had
another request... although I'm still not clear when that can happen.

[brianderson, 2015/10/29 22:38:28]

This looks okay to me. The main thread is blocked and can't add another
commit_completion_event. If it ever does happen, there's a DCHECK at the start
of ThreadProxy::StartCommitOnImpl that we'll hit. 

-        impl().commit_completion_event;
-    impl().commit_completion_event = nullptr;
   } else {
     impl().commit_completion_event->Signal();
     impl().commit_completion_event = nullptr;
@@ -951,19 +944,21 @@ void ThreadProxy::SetAnimationEvents(scoped_ptr<AnimationEventsVector> events) {
   layer_tree_host()->SetAnimationEvents(events.Pass());
 }
 
-void ThreadProxy::InitializeImplOnImplThread(CompletionEvent* completion) {
+void ThreadProxy::InitializeImplOnImpl(CompletionEvent* completion,
+                                       LayerTreeHost* layer_tree_host) {
   TRACE_EVENT0("cc", "ThreadProxy::InitializeImplOnImplThread");
   DCHECK(IsImplThread());
+  DCHECK(IsMainThreadBlocked());
+  DCHECK(layer_tree_host);
 
   // TODO(khushalsagar): ThreadedChannel will create ProxyImpl here and pass a
   // reference to itself.
   impl().channel_impl = threaded_channel_.get();
 
-  impl().layer_tree_host_impl =
-      layer_tree_host()->CreateLayerTreeHostImpl(this);
+  impl().layer_tree_host_impl = layer_tree_host->CreateLayerTreeHostImpl(this);
 
   SchedulerSettings scheduler_settings(
-      layer_tree_host()->settings().ToSchedulerSettings());
+      layer_tree_host->settings().ToSchedulerSettings());
 
   scoped_ptr<CompositorTimingHistory> compositor_timing_history(
       new CompositorTimingHistory(CompositorTimingHistory::RENDERER_UMA,
@@ -1020,7 +1015,7 @@ void ThreadProxy::FinishGLOnImpl(CompletionEvent* completion) {
   completion->Signal();
 }
 
-void ThreadProxy::LayerTreeHostClosedOnImplThread(CompletionEvent* completion) {
+void ThreadProxy::LayerTreeHostClosedOnImpl(CompletionEvent* completion) {
   TRACE_EVENT0("cc", "ThreadProxy::LayerTreeHostClosedOnImplThread");
   DCHECK(IsImplThread());
   DCHECK(IsMainThreadBlocked());
@@ -1124,11 +1119,13 @@ void ThreadProxy::DidActivateSyncTree() {
   TRACE_EVENT0("cc", "ThreadProxy::DidActivateSyncTreeOnImplThread");
   DCHECK(IsImplThread());
 
-  if (impl().completion_event_for_commit_held_on_tree_activation) {
+  if (impl().next_commit_waits_for_activation) {
     TRACE_EVENT_INSTANT0(
         "cc", "ReleaseCommitbyActivation", TRACE_EVENT_SCOPE_THREAD);
-    impl().completion_event_for_commit_held_on_tree_activation->Signal();
-    impl().completion_event_for_commit_held_on_tree_activation = nullptr;
+    DCHECK(impl().commit_completion_event);
+    impl().commit_completion_event->Signal();
+    impl().commit_completion_event = nullptr;
+    impl().next_commit_waits_for_activation = false;
   }
 
   impl().last_processed_begin_main_frame_args =