workers: Move ownership of WebThread from WorkerThread

workers: Move ownership of WebThread from WorkerThread

WorkerThread currently creates and owns the WebThread that is used for the
worker. However, for compositor-workers, we want to run all the worker scripts
in the same thread. To allow for this, move ownership of WebThread from
WorkerThread, and have each type of worker implementation provide the WebThread
to use for the worker.

BUG=436952, 436932
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194715

[sadrul, 2015-04-24 08:21:24]

sadrul@chromium.org changed reviewers:
+ jochen@chromium.org, kinuko@chromium.org

[kinuko, 2015-04-27 06:02:58]

https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:169: OwnPtr<WebThreadSupportingGC>
m_ownThread;
The relationship between m_thread and m_ownThread seems confusing, would it be
like the first WebThread creates a thread and owns it, and other WebThread's
will be reusing it? Can you briefly explain when and how and when a 'real'
thread is created and managed? Could we not to have both m_thread and
m_ownThread?

Also (as is mentioned in some other thread before) the name 'WorkerThread' is
becoming very confusing if it doesn't really mean a thread.

[sadrul, 2015-04-27 07:05:30]

https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:169: OwnPtr<WebThreadSupportingGC>
m_ownThread;
On 2015/04/27 06:02:58, kinuko wrote:
> The relationship between m_thread and m_ownThread seems confusing, would it be
> like the first WebThread creates a thread and owns it, and other WebThread's
> will be reusing it? Can you briefly explain when and how and when a 'real'
> thread is created and managed?

The idea is that for compositor workers, there will be a single
CompositorWorkerManager, which is responsible for managing the single
WebThreadSupportingGC used for all the CompositorWorkerThreads. So none of the
CompositorWorkerThreads actually owns the WebThread* instance,
CompositorWorkerManager does.

> Could we not to have both m_thread and
> m_ownThread?

One option (in fact the option I went with initially) is to move the ownership
of the WebThreadSupportingGC out of WorkerThread. For shared/dedicated/service
WorkerThread implementations, the corresponding sub-class would create and own
the WebThreadSupportingGC, and WorkerThread would access that through a virtual
thread() function. That would also allow CompositorWorkerThreads to share the
same WebThreadSupportingGC. Would that make this clearer?

> Also (as is mentioned in some other thread before) the name 'WorkerThread' is
> becoming very confusing if it doesn't really mean a thread.

Perhaps WorkerScript would be more appropriate?

[kinuko, 2015-04-27 11:14:46]

On 2015/04/27 07:05:30, sadrul wrote:
>
https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
> File Source/core/workers/WorkerThread.h (right):
> 
>
https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
> Source/core/workers/WorkerThread.h:169: OwnPtr<WebThreadSupportingGC>
> m_ownThread;
> On 2015/04/27 06:02:58, kinuko wrote:
> > The relationship between m_thread and m_ownThread seems confusing, would it
be
> > like the first WebThread creates a thread and owns it, and other WebThread's
> > will be reusing it? Can you briefly explain when and how and when a 'real'
> > thread is created and managed?
> 
> The idea is that for compositor workers, there will be a single
> CompositorWorkerManager, which is responsible for managing the single
> WebThreadSupportingGC used for all the CompositorWorkerThreads. So none of the
> CompositorWorkerThreads actually owns the WebThread* instance,
> CompositorWorkerManager does.

I see thanks thanks for the explanation.
 
> > Could we not to have both m_thread and
> > m_ownThread?
> 
> One option (in fact the option I went with initially) is to move the ownership
> of the WebThreadSupportingGC out of WorkerThread. For shared/dedicated/service
> WorkerThread implementations, the corresponding sub-class would create and own
> the WebThreadSupportingGC, and WorkerThread would access that through a
virtual
> thread() function. That would also allow CompositorWorkerThreads to share the
> same WebThreadSupportingGC. Would that make this clearer?

Hmm, yes I think that'd be probably clearer.

> > Also (as is mentioned in some other thread before) the name 'WorkerThread'
is
> > becoming very confusing if it doesn't really mean a thread.
> 
> Perhaps WorkerScript would be more appropriate?

Yeah, sounds better.

[sadrul, 2015-04-28 23:02:51]

On 2015/04/27 11:14:46, kinuko wrote:
> On 2015/04/27 07:05:30, sadrul wrote:
> >
>
https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
> > File Source/core/workers/WorkerThread.h (right):
> > 
> >
>
https://codereview.chromium.org/1100413004/diff/20001/Source/core/workers/Wor...
> > Source/core/workers/WorkerThread.h:169: OwnPtr<WebThreadSupportingGC>
> > m_ownThread;
> > On 2015/04/27 06:02:58, kinuko wrote:
> > > The relationship between m_thread and m_ownThread seems confusing, would
it
> be
> > > like the first WebThread creates a thread and owns it, and other
WebThread's
> > > will be reusing it? Can you briefly explain when and how and when a 'real'
> > > thread is created and managed?
> > 
> > The idea is that for compositor workers, there will be a single
> > CompositorWorkerManager, which is responsible for managing the single
> > WebThreadSupportingGC used for all the CompositorWorkerThreads. So none of
the
> > CompositorWorkerThreads actually owns the WebThread* instance,
> > CompositorWorkerManager does.
> 
> I see thanks thanks for the explanation.
>  
> > > Could we not to have both m_thread and
> > > m_ownThread?
> > 
> > One option (in fact the option I went with initially) is to move the
ownership
> > of the WebThreadSupportingGC out of WorkerThread. For
shared/dedicated/service
> > WorkerThread implementations, the corresponding sub-class would create and
own
> > the WebThreadSupportingGC, and WorkerThread would access that through a
> virtual
> > thread() function. That would also allow CompositorWorkerThreads to share
the
> > same WebThreadSupportingGC. Would that make this clearer?
> 
> Hmm, yes I think that'd be probably clearer.

Done.

> 
> > > Also (as is mentioned in some other thread before) the name 'WorkerThread'
> is
> > > becoming very confusing if it doesn't really mean a thread.
> > 
> > Perhaps WorkerScript would be more appropriate?
> 
> Yeah, sounds better.

I left that to do as a follow up after this CL (since it's pretty large and
makes it difficult to see the ownership changes in this CL). To clarify: we
agree to rename WorkerThread to WorkerScript, but not rename the various
*WorkerThread subclasses, since those currently are in fact responsible for
managing the threads. Right?

[sadrul, 2015-04-29 00:22:26]

> > > > Also (as is mentioned in some other thread before) the name
'WorkerThread'
> > is
> > > > becoming very confusing if it doesn't really mean a thread.
> > > 
> > > Perhaps WorkerScript would be more appropriate?
> > 
> > Yeah, sounds better.
> 
> I left that to do as a follow up after this CL (since it's pretty large and
> makes it difficult to see the ownership changes in this CL). To clarify: we
> agree to rename WorkerThread to WorkerScript, but not rename the various
> *WorkerThread subclasses, since those currently are in fact responsible for
> managing the threads. Right?

Although, as I think about this a bit more, I think it also makes good sense to
rename SharedWorkerThread, DedicatedWorkerThread et. al. to SharedWorkerScript,
DedicatedWorkerScript too. WDYT?

[kinuko, 2015-04-30 02:30:07]

Yes, if we rename WorkerThread to WorkerScript we'd better renaming all
*WorkerThread at once. (We may want to take one more look to see how it'd look
and if it looks to make sense)

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Ded...
File Source/core/workers/DedicatedWorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Ded...
Source/core/workers/DedicatedWorkerThread.h:50: WebThreadSupportingGC*
webThreadSupportingGC() override;
backingThread() may give better context?

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:286: m_thread->postTask(FROM_HERE, new
Task(threadSafeBind(&WorkerThread::initialize, AllowCrossThreadAccess(this))));
Can we have a separate flag (like m_started) and just use backingThread()
instead of holding a pointer in WorkerThread?

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:60: class CORE_EXPORT WorkerThread : public
RefCounted<WorkerThread> {
TODO for renaming this?

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:67: virtual WebThreadSupportingGC*
webThreadSupportingGC() = 0;
Can you give a brief explanation? It'd be nice to cover that: 1. it may create a
new thread on first access, and also 2. some implementation may reuse the
backing thread for multiple WorkerThread instance while most others do
one-to-one mapping.

[sadrul, 2015-04-30 05:27:40]

On 2015/04/30 02:30:07, kinuko wrote:
> Yes, if we rename WorkerThread to WorkerScript we'd better renaming all
> *WorkerThread at once. (We may want to take one more look to see how it'd look
> and if it looks to make sense)

https://codereview.chromium.org/1115923002 is a wip patch (on top of this CL) to
rename WorkerThread to WorkerScript. The change is mostly trivial, but touches a
lot of files.

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Ded...
File Source/core/workers/DedicatedWorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Ded...
Source/core/workers/DedicatedWorkerThread.h:50: WebThreadSupportingGC*
webThreadSupportingGC() override;
On 2015/04/30 02:30:07, kinuko wrote:
> backingThread() may give better context?

Agree. Done.

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:286: m_thread->postTask(FROM_HERE, new
Task(threadSafeBind(&WorkerThread::initialize, AllowCrossThreadAccess(this))));
On 2015/04/30 02:30:07, kinuko wrote:
> Can we have a separate flag (like m_started) and just use backingThread()
> instead of holding a pointer in WorkerThread?

Done.

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:60: class CORE_EXPORT WorkerThread : public
RefCounted<WorkerThread> {
On 2015/04/30 02:30:07, kinuko wrote:
> TODO for renaming this?

Done.

https://codereview.chromium.org/1100413004/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:67: virtual WebThreadSupportingGC*
webThreadSupportingGC() = 0;
On 2015/04/30 02:30:07, kinuko wrote:
> Can you give a brief explanation? It'd be nice to cover that: 1. it may create
a
> new thread on first access, and also 2. some implementation may reuse the
> backing thread for multiple WorkerThread instance while most others do
> one-to-one mapping.

Done.

[kinuko, 2015-04-30 05:31:19]

lgtm

https://codereview.chromium.org/1100413004/diff/60001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/60001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:72: virtual WebThreadSupportingGC*
backingThread() = 0;
If this never returns nullptr we might want to use a reference instead?

[sadrul, 2015-04-30 06:14:56]

https://codereview.chromium.org/1100413004/diff/60001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1100413004/diff/60001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:72: virtual WebThreadSupportingGC*
backingThread() = 0;
On 2015/04/30 05:31:18, kinuko wrote:
> If this never returns nullptr we might want to use a reference instead?

Good point. Done.

[jochen (gone - plz use gerrit), 2015-04-30 07:28:25]

rubberstamp lgtm

[sadrul, 2015-04-30 07:38:33]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-04-30 07:38:34]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1100413004/#ps80001
(title: "reference")

[commit-bot: I haz the power, 2015-04-30 07:39:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100413004/80001

[commit-bot: I haz the power, 2015-04-30 09:19:05]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194715