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

cc/trees/thread_proxy.cc

 // Copyright 2011 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/trees/thread_proxy.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 44 matching lines @@
                                          impl_task_runner,
                                          external_begin_frame_source.Pass()));
 }
 
 ThreadProxy::ThreadProxy(
     LayerTreeHost* layer_tree_host,
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     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(),
           layer_tree_host->rendering_stats_instrumentation(),
           external_begin_frame_source.Pass()) {
   TRACE_EVENT0("cc", "ThreadProxy::ThreadProxy");
   DCHECK(IsMainThread());
   DCHECK(this->layer_tree_host());
   // TODO(khushalsagar): Move this to LayerTreeHost#InitializeThreaded once
   // ThreadProxy is split. LayerTreeHost creates the channel and passes it to
   // ProxyMain#SetChannel.
   SetChannel(ThreadedChannel::Create(this, main_task_runner, impl_task_runner));
 }
 
 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),
       weak_factory(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,
     int layer_tree_host_id,
     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),
       smoothness_priority_expiration_notifier(
           proxy->ImplThreadTaskRunner(),
           base::Bind(&ThreadProxy::RenewTreePriority, base::Unretained(proxy)),
           base::TimeDelta::FromMilliseconds(
               kSmoothnessTakesPriorityExpirationDelay * 1000)),
       external_begin_frame_source(external_begin_frame_source.Pass()),
       rendering_stats_instrumentation(rendering_stats_instrumentation),
@@ skipping 281 matching lines @@
   main().channel_main->SetNeedsRedrawOnImpl(damage_rect);
 }
 
 void ThreadProxy::SetNeedsRedrawOnImpl(const gfx::Rect& damage_rect) {
   DCHECK(IsImplThread());
   SetNeedsRedrawRectOnImplThread(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) {
   DCHECK(IsMainThread());
   if (main().defer_commits == defer_commits)
     return;
 
   main().defer_commits = defer_commits;
   if (main().defer_commits)
     TRACE_EVENT_ASYNC_BEGIN0("cc", "ThreadProxy::SetDeferCommits", this);
@@ skipping 60 matching lines @@
 }
 
 void ThreadProxy::SetInputThrottledUntilCommitOnImpl(bool is_throttled) {
   DCHECK(IsImplThread());
   if (is_throttled == impl().input_throttled_until_commit)
     return;
   impl().input_throttled_until_commit = is_throttled;
   RenewTreePriority();
 }
 
 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() {
   DCHECK(IsMainThread());
   return main_thread_only_vars_unsafe_;
 }
 const ThreadProxy::MainThreadOnly& ThreadProxy::main() const {
   DCHECK(IsMainThread());
   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() {
   DCHECK(IsImplThread());
   return compositor_thread_vars_unsafe_;
 }
 
 const ThreadProxy::CompositorThreadOnly& ThreadProxy::impl() const {
   DCHECK(IsImplThread());
   return compositor_thread_vars_unsafe_;
 }
 
 void ThreadProxy::Start() {
   DCHECK(IsMainThread());
   DCHECK(Proxy::HasImplThread());
 
   // 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();
 
   main().started = true;
 }
 
 void ThreadProxy::Stop() {
   TRACE_EVENT0("cc", "ThreadProxy::Stop");
   DCHECK(IsMainThread());
   DCHECK(main().started);
 
   // Synchronously finishes pending GL operations and deletes the impl.
   // The two steps are done as separate post tasks, so that tasks posted
   // by the GL implementation due to the Finish can be executed by the
   // renderer before shutting it down.
   {
     DebugScopedSetMainThreadBlocked main_thread_blocked(this);
     CompletionEvent completion;
     main().channel_main->FinishGLOnImpl(&completion);
     completion.Wait();
   }
   {
     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;
 }
 
 bool ThreadProxy::SupportsImplScrolling() const {
   return true;
 }
 
 void ThreadProxy::FinishAllRenderingOnImpl(CompletionEvent* completion) {
   TRACE_EVENT0("cc", "ThreadProxy::FinishAllRenderingOnImplThread");
   DCHECK(IsImplThread());
@@ skipping 125 matching lines @@
     TRACE_EVENT0("cc", "ThreadProxy::BeginMainFrame::commit");
 
     DebugScopedSetMainThreadBlocked main_thread_blocked(this);
 
     // This CapturePostTasks should be destroyed before CommitComplete() is
     // called since that goes out to the embedder, and we want the embedder
     // to receive its callbacks before that.
     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();
   }
 
   main().current_pipeline_stage = NO_PIPELINE_STAGE;
   layer_tree_host()->CommitComplete();
   layer_tree_host()->DidBeginMainFrame();
 }
 
 void ThreadProxy::BeginMainFrameNotExpectedSoon() {
   TRACE_EVENT0("cc", "ThreadProxy::BeginMainFrameNotExpectedSoon");
   DCHECK(IsMainThread());
   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);
     completion->Signal();
     return;
   }
 
   // Ideally, we should inform to impl thread when BeginMainFrame is started.
   // 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();
 }
 
 void ThreadProxy::BeginMainFrameAbortedOnImpl(CommitEarlyOutReason reason) {
   TRACE_EVENT1("cc", "ThreadProxy::BeginMainFrameAbortedOnImplThread", "reason",
                CommitEarlyOutReasonToString(reason));
   DCHECK(IsImplThread());
   DCHECK(impl().scheduler);
   DCHECK(impl().scheduler->CommitPending());
   DCHECK(!impl().layer_tree_host_impl->pending_tree());
@@ skipping 12 matching lines @@
   DCHECK(IsImplThread());
 
   impl().layer_tree_host_impl->Animate();
 }
 
 void ThreadProxy::ScheduledActionCommit() {
   TRACE_EVENT0("cc", "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;
   }
 
   impl().scheduler->DidCommit();
 
   // Delay this step until afer the main thread has been released as it's
   // often a good bit of work to update the tree and prepare the new frame.
   impl().layer_tree_host_impl->CommitComplete();
@@ skipping 126 matching lines @@
   DCHECK(IsMainThread());
   layer_tree_host()->DidCompleteSwapBuffers();
 }
 
 void ThreadProxy::SetAnimationEvents(scoped_ptr<AnimationEventsVector> events) {
   TRACE_EVENT0("cc", "ThreadProxy::SetAnimationEvents");
   DCHECK(IsMainThread());
   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,
                                   impl().rendering_stats_instrumentation));
 
   impl().scheduler = Scheduler::Create(
       this, scheduler_settings, impl().layer_tree_host_id,
       ImplThreadTaskRunner(), impl().external_begin_frame_source.get(),
       compositor_timing_history.Pass());
 
@@ skipping 36 matching lines @@
   DCHECK(IsImplThread());
   if (impl().layer_tree_host_impl->output_surface()) {
     ContextProvider* context_provider =
         impl().layer_tree_host_impl->output_surface()->context_provider();
     if (context_provider)
       context_provider->ContextGL()->Finish();
   }
   completion->Signal();
 }
 
-void ThreadProxy::LayerTreeHostClosedOnImplThread(CompletionEvent* completion) {
+void ThreadProxy::LayerTreeHostClosedOnImpl(CompletionEvent* completion) {
   TRACE_EVENT0("cc", "ThreadProxy::LayerTreeHostClosedOnImplThread");
   DCHECK(IsImplThread());
   DCHECK(IsMainThreadBlocked());
   impl().scheduler = nullptr;
   impl().external_begin_frame_source = nullptr;
   impl().layer_tree_host_impl = nullptr;
   impl().weak_factory.InvalidateWeakPtrs();
   // We need to explicitly shutdown the notifier to destroy any weakptrs it is
   // holding while still on the compositor thread. This also ensures any
   // callbacks holding a ThreadProxy pointer are cancelled.
@@ skipping 83 matching lines @@
 void ThreadProxy::PostDelayedAnimationTaskOnImplThread(
     const base::Closure& task,
     base::TimeDelta delay) {
   Proxy::ImplThreadTaskRunner()->PostDelayedTask(FROM_HERE, task, delay);
 }
 
 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 =
       impl().last_begin_main_frame_args;
 }
 
 void ThreadProxy::WillPrepareTiles() {
   DCHECK(IsImplThread());
   impl().scheduler->WillPrepareTiles();
 }
@@ skipping 46 matching lines @@
 
 base::WeakPtr<ProxyMain> ThreadProxy::GetMainWeakPtr() {
   return main_thread_weak_ptr_;
 }
 
 base::WeakPtr<ProxyImpl> ThreadProxy::GetImplWeakPtr() {
   return impl_thread_weak_ptr_;
 }
 
 }  // namespace cc