compositor-worker: Refactor CompositorWorkerManager

content/child/blink_platform_impl.cc

 // Copyright 2014 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 "content/child/blink_platform_impl.h"
 
 #include <math.h>
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/rand_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #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"
 #include "base/trace_event/trace_event.h"
 #include "blink/public/resources/grit/blink_image_resources.h"
 #include "blink/public/resources/grit/blink_resources.h"
 #include "components/mime_util/mime_util.h"
 #include "components/scheduler/child/web_task_runner_impl.h"
 #include "components/scheduler/child/webthread_impl_for_worker_scheduler.h"
 #include "content/app/resources/grit/content_resources.h"
@@ skipping 397 matching lines @@
 // TODO(skyostil): Ensure that we always have an active task runner when
 // constructing the platform.
 BlinkPlatformImpl::BlinkPlatformImpl()
     : BlinkPlatformImpl(base::ThreadTaskRunnerHandle::IsSet()
                             ? base::ThreadTaskRunnerHandle::Get()
                             : nullptr) {
 }
 
 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();
 }
 
 void BlinkPlatformImpl::InternalInit() {
   // ChildThread may not exist in some tests.
   if (ChildThreadImpl::current()) {
     geofencing_provider_.reset(new WebGeofencingProviderImpl(
         ChildThreadImpl::current()->thread_safe_sender()));
     thread_safe_sender_ = ChildThreadImpl::current()->thread_safe_sender();
     notification_dispatcher_ =
@@ skipping 66 matching lines @@
 bool BlinkPlatformImpl::portAllowed(const blink::WebURL& url) const {
   GURL gurl = GURL(url);
   // Return true for URLs without a port specified.  This is needed to let
   // through non-network schemes that don't go over the network.
   if (!gurl.has_port())
     return true;
   return net::IsPortAllowedForScheme(gurl.EffectiveIntPort(), gurl.scheme());
 }
 
 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());
 }
 
 void BlinkPlatformImpl::yieldCurrentThread() {
   base::PlatformThread::YieldCurrentThread();
 }
 
 blink::WebWaitableEvent* BlinkPlatformImpl::createWaitableEvent(
     blink::WebWaitableEvent::ResetPolicy policy,
@@ skipping 532 matching lines @@
 
 double BlinkPlatformImpl::systemTraceTime() {
   return (base::TimeTicks::Now() - base::TimeTicks()).InSecondsF();
 }
 
 void BlinkPlatformImpl::cryptographicallyRandomValues(
     unsigned char* buffer, size_t length) {
   base::RandBytes(buffer, length);
 }
 
+blink::WebThread* BlinkPlatformImpl::compositorThread() const {
+  return compositor_thread_;
+}
+
 blink::WebGestureCurve* BlinkPlatformImpl::createFlingAnimationCurve(
     blink::WebGestureDevice device_source,
     const blink::WebFloatPoint& velocity,
     const blink::WebSize& cumulative_scroll) {
   return ui::WebGestureCurveImpl::CreateFromDefaultPlatformCurve(
              gfx::Vector2dF(velocity.x, velocity.y),
              gfx::Vector2dF(cumulative_scroll.width, cumulative_scroll.height),
              IsMainThread()).release();
 }
 
@@ skipping 196 matching lines @@
   return WebString::fromUTF8(ui::KeycodeConverter::DomKeyToKeyString(
       static_cast<ui::DomKey>(dom_key)));
 }
 
 int BlinkPlatformImpl::domKeyEnumFromString(const WebString& key_string) {
   return static_cast<int>(
       ui::KeycodeConverter::KeyStringToDomKey(key_string.utf8()));
 }
 
 }  // namespace content