compositor-worker: Share a thread and an isolate for compositor workers.

Source/modules/compositorworker/CompositorWorkerManager.cpp

Index: Source/modules/compositorworker/CompositorWorkerManager.cpp
diff --git a/Source/modules/compositorworker/CompositorWorkerManager.cpp b/Source/modules/compositorworker/CompositorWorkerManager.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..dc7fd62952dbcae3837c6ab1c628aa83409e4ddd
--- /dev/null
+++ b/Source/modules/compositorworker/CompositorWorkerManager.cpp
@@ -0,0 +1,158 @@
+// Copyright 2015 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 "config.h"
+#include "modules/compositorworker/CompositorWorkerManager.h"
+
+#include "platform/ThreadSafeFunctional.h"
+#include "platform/WebThreadSupportingGC.h"
+#include "wtf/MainThread.h"
+#include "wtf/ThreadingPrimitives.h"
+
+namespace blink {
+
+namespace {
+
+class SharedV8Isolate final : public WorkerIsolateWrapper {

[haraken, 2015/06/02 02:14:49]

I think this is mis-designed and complicated more than needed. You now have
WorkerIsolateWrapper, WorkerIsolateWrapperDefault, SharedV8Isolate,
CompositorWorkerManager (which is doing a reference counting for worker
threads).

In particular, I don't quite understand why you need SharedV8Isolate. Why can't
we simply create an backing Isolate when CompositorWorkerManager creates the
first worker and destroy the Isolate when CompositorWorkerManager destroys the
last worker? CompositorWorkerManager should know the timing when the backing
Isolate should be created or destroyed without getting any help of
SharedV8Isolate etc.

I think WorkerIsolateWrapper is a good abstraction, but I'm not sure if we need
WorkerIsolateWrapperDefault and SharedV8Isolate.

[sadrul, 2015/06/02 02:37:53]

We need SharedV8Isolate because each WorkerThread owns its own
WorkerIsolateWrapper instance. For compositor workers, we cannot pass ownership
of the isolate to the first worker, since we want the manager to retain
ownership. So we need a version of WorkerIsolateWrapper that doesn't take
ownership of the isolate. SharedV8Isolate+CompositorWorkerManager essentially
does as you suggest (by creating an isolate for the first worker, and destroying
when the last worker is terminated), but I suppose through some indirection.
Some amount of code-rearrangement can make this better by moving the ownership
out of WorkerThread (into the subclass implementations, for example), and have
WorkerThread work with a raw-pointer that it receives from the subclass instead.
This way, we won't need SharedV8Isolate. Would that me more palatable?

[haraken, 2015/06/02 02:42:23]

Just to confirm:

Compositor workers : CompositorWorkerManager : WorkerThread : Isolate = N : 1 :
1 : 1

Maybe am I misunderstanding?

[sadrul, 2015/06/02 02:48:28]

There can be multiple WorkerThread instances. So:

CompositorWorkerManager : Isolate : Compositor workers : WorkerThread = 1 : 1 :
N : N

To clarify a bit more:

CompositorWorkerManager : Isolate = 1 : 1
CompositorWorkerManager : Compositor workers = 1 : N
Compositor worker : WorkerThread = 1 : 1

[haraken, 2015/06/02 03:34:26]

Thanks for the clarification!

It seems that the current class design doesn't reflect the concept. For example,
in this CL WorkerThread owns an Isolate, but conceptually there is no ownership
relationship from WorkerThread to Isolate. Would it be possible to reconsider
the class design that reflects the concept more straightforwardly?

[sadrul, 2015/06/02 04:23:56]

The WorkerThread doesn't necessarily own the Isolate. That is why we have this
IsolateWrapper, which may or may not own the isolate. The IsolateWrapper for the
CompositorWorkerThread does not own the isolate, since the isolate is shared
with other CompositorWorkerThread instances. For other workers (e.g.
DedicatedWorkerThread, ServiceWorkerThread, SharedWorkerThread), the
IsolateWrapper used (the default one) does own the Isolate. So essentially, each
WorkerThread owns an IsolateWrapper to communicate with the isolate, but the
ownership of the v8::Isolate is left up to the implementation (i.e. the
subclass) to decide. (I have attempted to clarify this for WorkerIsolateWrapper
doc here:
https://codereview.chromium.org/1158443008/diff/240001/Source/core/workers/Wo...)

To briefly explain: each worker is represented by a WorkerThread (which is a bit
of a misnomer; we still intend to rename it to WorkerScript:
https://codereview.chromium.org/1115923002/), which has a WorkerGlobalScope,
which owns a WorkerScriptController. We do need a WorkerThread for each
compositor worker (because we want each worker to have its own global-scope),
and because all of them live in the same thread, it is also necessary for these
workers to share the v8::Isolate. So the ownership of the v8::Isolate does have
to be different for compositor-workers from the other workers, and this seems
fairly unavoidable. Does this make a reasonable amount of sense to explain the
isolate ownership?

(Note: we do have plans on experimenting with multiple threads/isolates, and
this design does allow for that to happen fairly easily, but we want to start
with the simpler approach)

+    WTF_MAKE_NONCOPYABLE(SharedV8Isolate);
+public:
+    explicit SharedV8Isolate(v8::Isolate* isolate)
+        : m_isolate(isolate) { }
+    ~SharedV8Isolate() override
+    {
+        CompositorWorkerManager::instance()->destroyIsolate();
+    }
+
+private:
+    // WorkerIsolateWrapper:
+    v8::Isolate* isolate() const override { return m_isolate; }
+    void willDestroy() override
+    {
+        CompositorWorkerManager::instance()->willDestroyIsolate();
+    }
+
+    void terminateExecution() override
+    {
+        CompositorWorkerManager::instance()->terminateIsolate();
+    }
+
+    v8::Isolate* m_isolate;
+};
+
+static CompositorWorkerManager* s_instance = nullptr;
+
+static Mutex& singletonMutex()
+{
+    AtomicallyInitializedStaticReference(Mutex, mutex, new Mutex);
+    return mutex;
+}
+
+static void destroyThread(WebThreadSupportingGC* thread)

[haraken, 2015/06/02 02:14:49]

Where is this called?

[sadrul, 2015/06/02 02:37:53]

In a posted task (below in line 116).

+{
+    delete thread;

[haraken, 2015/06/02 02:14:49]

We should avoid calling manual delete...

[sadrul, 2015/06/02 02:37:53]

I tried using something like DeleteSoon() in chrome, but couldn't find something
similar for blink. Is there anything that I missed?

+}
+
+} // namespace
+
+void CompositorWorkerManager::initialize()
+{
+    MutexLocker lock(singletonMutex());
+    ASSERT(!s_instance);
+    s_instance = new CompositorWorkerManager();
+}
+
+void CompositorWorkerManager::shutdown()
+{
+    MutexLocker lock(singletonMutex());
+    ASSERT(s_instance);
+    delete s_instance;
+    s_instance = nullptr;
+}
+
+CompositorWorkerManager* CompositorWorkerManager::instance()
+{
+    MutexLocker lock(singletonMutex());
+    ASSERT(s_instance);
+    return s_instance;
+}
+
+CompositorWorkerManager::CompositorWorkerManager()
+{
+}
+
+CompositorWorkerManager::~CompositorWorkerManager()
+{
+}
+
+WebThreadSupportingGC& CompositorWorkerManager::compositorWorkerThread()
+{
+    MutexLocker lock(m_mutex);
+    if (!m_thread) {
+        ASSERT(isMainThread());
+        ASSERT(!m_workerCount);
+        m_thread = WebThreadSupportingGC::create("CompositorWorker Thread");
+    }
+    return *m_thread.get();
+}
+
+void CompositorWorkerManager::initializeBackingThread()
+{
+    ASSERT(m_thread->isCurrentThread());
+    MutexLocker lock(m_mutex);
+    ++m_workerCount;
+    if (m_workerCount == 1)
+        m_thread->initialize();
+}
+
+void CompositorWorkerManager::shutdownBackingThread()
+{
+    MutexLocker lock(m_mutex);
+    ASSERT(m_thread->isCurrentThread());
+    ASSERT(m_workerCount > 0);
+    --m_workerCount;
+    if (m_workerCount == 0) {
+        m_thread->shutdown();
+        Platform::current()->mainThread()->postTask(FROM_HERE, threadSafeBind(destroyThread, AllowCrossThreadAccess(m_thread.leakPtr())));
+    }
+}
+
+PassOwnPtr<WorkerIsolateWrapper> CompositorWorkerManager::createIsolate()
+{
+    MutexLocker lock(m_mutex);
+    ASSERT(m_thread->isCurrentThread());
+    if (!m_isolate)
+        m_isolate = WorkerIsolateWrapper::createDefault();
+    // It is safe to use the existing isolate even if TerminateExecution() has been
+    // called on it, without calling CancelTerminateExecution().
+    return adoptPtr(new SharedV8Isolate(m_isolate->isolate()));
+}
+
+void CompositorWorkerManager::willDestroyIsolate()
+{
+    MutexLocker lock(m_mutex);
+    ASSERT(m_thread->isCurrentThread());
+    if (m_workerCount == 1)
+        m_isolate->willDestroy();
+}
+
+void CompositorWorkerManager::destroyIsolate()
+{
+    MutexLocker lock(m_mutex);
+    if (!m_thread) {
+        ASSERT(m_workerCount == 0);
+        m_isolate.clear();
+    }
+}
+
+void CompositorWorkerManager::terminateIsolate()
+{
+    MutexLocker lock(m_mutex);
+    ASSERT(isMainThread());
+    if (m_workerCount > 1)
+        return;
+
+    m_isolate->terminateExecution();
+}
+
+} // namespace blink