compositor-worker: Refactor CompositorWorkerManager

content/child/blink_platform_impl.cc

Index: content/child/blink_platform_impl.cc
diff --git a/content/child/blink_platform_impl.cc b/content/child/blink_platform_impl.cc
index ca621267cf83df6204699d1f2ef338b7ebf54f5c..1f05b21b957eced95221f949f14dfb31bb5a12e4 100644
--- a/content/child/blink_platform_impl.cc
+++ b/content/child/blink_platform_impl.cc
@@ -25,6 +25,7 @@
 #include "base/sys_info.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "base/trace_event/memory_allocator_dump_guid.h"
 #include "base/trace_event/memory_dump_manager.h"
@@ -442,7 +443,8 @@ BlinkPlatformImpl::BlinkPlatformImpl()
 
 BlinkPlatformImpl::BlinkPlatformImpl(
     scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner)
-    : main_thread_task_runner_(main_thread_task_runner) {
+    : main_thread_task_runner_(main_thread_task_runner),
+      compositor_thread_(nullptr) {
   InternalInit();
 }
 
@@ -529,14 +531,30 @@ bool BlinkPlatformImpl::portAllowed(const blink::WebURL& url) const {
 }
 
 blink::WebThread* BlinkPlatformImpl::createThread(const char* name) {
+  base::Thread::Options options;
   scheduler::WebThreadImplForWorkerScheduler* thread =
-      new scheduler::WebThreadImplForWorkerScheduler(name);
+      new scheduler::WebThreadImplForWorkerScheduler(name, options);
   thread->TaskRunner()->PostTask(
       FROM_HERE, base::Bind(&BlinkPlatformImpl::UpdateWebThreadTLS,
                             base::Unretained(this), thread));
   return thread;
 }
 
+blink::WebThread* BlinkPlatformImpl::createThreadForCompositor(
+    const char* name) {
+  base::Thread::Options options;
+  DCHECK(!compositor_thread_);
+#if defined(OS_ANDROID)
+  options.priority = base::ThreadPriority::DISPLAY;
+#endif
+  compositor_thread_ =
+      new scheduler::WebThreadImplForWorkerScheduler(name, options);

[piman, 2015/11/30 21:41:45]

I think this makes ownership muddy and confusing. In particular, the
Unretained(this) below is not obviously correct, since BlinkPlatformImpl doesn't
own the compositor_thread_.

I would prefer if the compositor thread was created by RenderThreadImpl and
injected into the constructor, the same way the main thread is.

[Ian Vollick, 2015/12/01 04:00:04]

Agree that this is crummy, but I think there's some circularity here. The
BlinkPlatformImpl instance is responsible for setting up the WebThreadTLS for
web worker threads. How would you feel about passing a
WebThreadImplForWorkerScheduler* to the constructor, but posting a task to run
BlinkPlatformImpl::UpdateWebThreadTLS from BlinkPlatformImpl::InternalInit? I
think I'd still need to have the base::Unretained(this) bit if I did that,
though.

[piman, 2015/12/01 04:24:10]

Unretained(this) is still fine if BlinkPlatformImpl synchronizes with the thread
before destruction, and the thread outlives BlinkPlatformImpl. The alternative
is to return a scoped_ptr<WebThreadImplForWorkerScheduler> (or
scoped_ptr<WebThreadBase>) and add a check somewhere that the thread is
correctly joined/destroyed before the BlinkPlatformImpl. Either way, we need a
guarantee that this Unretained is safe.

It's just extremely weird to have this thread owned by blink, when it's really a
choice of the embedder, which can otherwise run the compositor without any blink
interaction, and can even choose not to have a separate thread.

[Ian Vollick, 2015/12/01 05:01:08]

I think I've got a plan, but before describing it I want to mention that blink
only knows about the compositor thread, it doesn't own it. It should also be
possible in future to support running CompositorWorkers on the main thread if we
decide not to create a compositor thread, though in this patch I was just trying
to get the workers off of an entirely separate thread and onto the compositor
thread.

--

Here's what I intended to try tomorrow (please let me know if this won't address
your concerns).

1. Rather than have createThreadForCompositor hang onto a pointer to the
compositor thread as a side effect, I was going to replace that function with
createThreadWithOptions (so that we can set some options for the compositor
thread on Android). That new function will not hang onto a pointer; it will be
very similar to createThread.

2. I was then going to add BlinkPlatformImpl::SetCompositorThread(...) and make
it clear that BPI is not taking ownership of the pointer it is passed. Calls to
Platform::compositorThread will simply vend the pointer that was passed to this
function.

3. Before the call to SetCompositorThread in
RenderThreadImpl::EnsureWebKitInitialized, I was going to block on the
initialization of the compositor thread, since it should be very quick. This
should guarantee that the BlinkPlatformImpl instance is alive when the
UpdateWebThreadTLS task gets run.

4. I'll clear the compositor thread via SetCompositorThread before we shut it
down.

Does that sound reasonable?

+  compositor_thread_->TaskRunner()->PostTask(
+      FROM_HERE, base::Bind(&BlinkPlatformImpl::UpdateWebThreadTLS,
+                            base::Unretained(this), compositor_thread_));
+  return compositor_thread_;
+}
+
 blink::WebThread* BlinkPlatformImpl::currentThread() {
   return static_cast<blink::WebThread*>(current_thread_slot_.Get());
 }
@@ -1089,6 +1107,10 @@ void BlinkPlatformImpl::cryptographicallyRandomValues(
   base::RandBytes(buffer, length);
 }
 
+blink::WebThread* BlinkPlatformImpl::compositorThread() const {
+  return compositor_thread_;
+}
+
 blink::WebGestureCurve* BlinkPlatformImpl::createFlingAnimationCurve(
     blink::WebGestureDevice device_source,
     const blink::WebFloatPoint& velocity,