compositor-worker: Get the thread to run compositor-workers from the Platform.

workers: Slightly change how WorkerThread uses a WebThread.

Right now, a WorkerThread creates a WebThreadSupportingGC for each 
WebThread that runs worker code. So, in the case of normal workers (i.e.
shared/dedicated/service), each WorkerThread spins up a new 
WebThreadSupportingGC (which creates and owns a new WebThread instance).
For compositor-workers, a single WebThreadSupportingGC is created for all
workers by CompositorWorkerManager. This instance of WebThreadSupportingGC
creates and owns the thread that runs the compositor-workers.

However, we want to run the compositor workers in the same thread as the 
compositor. This compositor-thread is created, owned, and maintained by the
Platform. Therefore, it is not possible for CompositorWorkerManager to 
create a WebThreadSupportingGC instance. So, change the WorkerThread to
work directly with a WebThread. All the normal workers directly create a
WebThread instance, and WorkerThread manages the GC support for it. However,
CompositorWorkerManager directly creates both a WebThread instance, and
necessary code for adding support for GC (GCSupportForWebThread)
separately. In a subsequent CL, this will be changed so that instead of 
creating and owning the WebThread instance, the CompositorWorkerManager
gets the WebThread for running the workers from the Platform.

BUG=518708

[sadrul, 2015-08-10 03:38:39]

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

[kinuko, 2015-08-10 04:59:36]

kinuko@chromium.org changed reviewers:
+ toyoshim@chromium.org

[kinuko, 2015-08-10 05:01:21]

(+toyoshim@ for WorkerThread changes)

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

https://codereview.chromium.org/1274023003/diff/60001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:172: OwnPtr<GCSupportForWebThread>
m_gcSupport;
I'm a bit confused. Regular workers own WebThreadSupportingGC, which owns
GCSupport and WebThread, while CompositorWorker uses CompositorWorkerManager's
GCSupport and WebThread-- so it doesn't seem we should have the base class own
GCSupport but I'm missing something?

[sadrul, 2015-08-10 06:51:20]

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

https://codereview.chromium.org/1274023003/diff/60001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:172: OwnPtr<GCSupportForWebThread>
m_gcSupport;
On 2015/08/10 05:01:21, kinuko wrote:
> I'm a bit confused. Regular workers own WebThreadSupportingGC, which owns
> GCSupport and WebThread, while CompositorWorker uses CompositorWorkerManager's
> GCSupport and WebThread-- so it doesn't seem we should have the base class own
> GCSupport but I'm missing something?

Oh crud. You are right. Sorry, I meant to clean this up before sending off for
review, but forgot.

I have changed the workers to directly create a WebThread instead, and kept the
gc-support management in WorkerThread, so that all the normal workers don't have
to override initializeBackingThread() and shutdownBackingThread() (this
management applies for the normal workers, but compositor-worker overrides this
behaviour; so CompositorWorkerManager still has its own gc-support).

[kinuko, 2015-08-10 14:10:06]

I think I prefer regular workers continue to have WebThreadSupportingGC rather
than decoupling it only for coding issues.  I wonder if we could add one more
create() method to WebThreadSupportingGC that takes an existing thread, so that
we can create WebThreadSupportingGC for a given thread.  And the WebThread
implementation for the compositor thread can be probably made in a way that
destructing the WebThread instance doesn't mean stopping the underlying thread. 
Then we can consistently use WebThreadSupportingGC in all worker thread code.
Would something like that work / make sense?

[haraken, 2015-08-10 14:23:37]

On 2015/08/10 14:10:06, kinuko wrote:
> I think I prefer regular workers continue to have WebThreadSupportingGC rather
> than decoupling it only for coding issues.  I wonder if we could add one more
> create() method to WebThreadSupportingGC that takes an existing thread, so
that
> we can create WebThreadSupportingGC for a given thread.  And the WebThread
> implementation for the compositor thread can be probably made in a way that
> destructing the WebThread instance doesn't mean stopping the underlying
thread. 
> Then we can consistently use WebThreadSupportingGC in all worker thread code.
> Would something like that work / make sense?

Sounds like a plan!

[sadrul, 2015-08-10 14:36:07]

On 2015/08/10 14:23:37, haraken wrote:
> On 2015/08/10 14:10:06, kinuko wrote:
> > I think I prefer regular workers continue to have WebThreadSupportingGC
rather
> > than decoupling it only for coding issues.  I wonder if we could add one
more
> > create() method to WebThreadSupportingGC that takes an existing thread, so
> that
> > we can create WebThreadSupportingGC for a given thread.  And the WebThread
> > implementation for the compositor thread can be probably made in a way that
> > destructing the WebThread instance doesn't mean stopping the underlying
> thread. 
> > Then we can consistently use WebThreadSupportingGC in all worker thread
code.
> > Would something like that work / make sense?
> 
> Sounds like a plan!

I actually considered doing that at first. That way, there's little change to be
made in blink. However, WorkerThread deals with both the WebThread, and its
corresponding WebScheduler. So the Platform implementation will need to create
proxies for both of these, and it becomes ugly pretty quickly.

Is there a particular reason you think having the workers continue to create
WebThreadSupportingGC is beneficial? We can still make that the case (and
override initializeBackingThread()/shutdownBackingThread()) if you think that is
better.

[kinuko, 2015-08-11 04:36:11]

On 2015/08/10 14:36:07, sadrul wrote:
> On 2015/08/10 14:23:37, haraken wrote:
> > On 2015/08/10 14:10:06, kinuko wrote:
> > > I think I prefer regular workers continue to have WebThreadSupportingGC
> rather
> > > than decoupling it only for coding issues.  I wonder if we could add one
> more
> > > create() method to WebThreadSupportingGC that takes an existing thread, so
> > that
> > > we can create WebThreadSupportingGC for a given thread.  And the WebThread
> > > implementation for the compositor thread can be probably made in a way
that
> > > destructing the WebThread instance doesn't mean stopping the underlying
> > thread. 
> > > Then we can consistently use WebThreadSupportingGC in all worker thread
> code.
> > > Would something like that work / make sense?
> > 
> > Sounds like a plan!
> 
> I actually considered doing that at first. That way, there's little change to
be
> made in blink. However, WorkerThread deals with both the WebThread, and its
> corresponding WebScheduler. So the Platform implementation will need to create
> proxies for both of these, and it becomes ugly pretty quickly.
> 
> Is there a particular reason you think having the workers continue to create
> WebThreadSupportingGC is beneficial? We can still make that the case (and
> override initializeBackingThread()/shutdownBackingThread()) if you think that
is
> better.

I think I worry that decoupling GC support and WebThread could make the code
error-prone. We're going to have three combinations of GC support and WebThread
usage and every one's going to be implemented in a different file, which
concerns me a bit.

How would it look to you/haraken if we also add detachThread() or something to
WebThreadSupportingGC, which detaches the associated WebThread from
WebThreadSupportingGC?

[sadrul, 2015-08-11 04:45:29]

On 2015/08/11 04:36:11, kinuko wrote:
> On 2015/08/10 14:36:07, sadrul wrote:
> > On 2015/08/10 14:23:37, haraken wrote:
> > > On 2015/08/10 14:10:06, kinuko wrote:
> > > > I think I prefer regular workers continue to have WebThreadSupportingGC
> > rather
> > > > than decoupling it only for coding issues.  I wonder if we could add one
> > more
> > > > create() method to WebThreadSupportingGC that takes an existing thread,
so
> > > that
> > > > we can create WebThreadSupportingGC for a given thread.  And the
WebThread
> > > > implementation for the compositor thread can be probably made in a way
> that
> > > > destructing the WebThread instance doesn't mean stopping the underlying
> > > thread. 
> > > > Then we can consistently use WebThreadSupportingGC in all worker thread
> > code.
> > > > Would something like that work / make sense?
> > > 
> > > Sounds like a plan!
> > 
> > I actually considered doing that at first. That way, there's little change
to
> be
> > made in blink. However, WorkerThread deals with both the WebThread, and its
> > corresponding WebScheduler. So the Platform implementation will need to
create
> > proxies for both of these, and it becomes ugly pretty quickly.
> > 
> > Is there a particular reason you think having the workers continue to create
> > WebThreadSupportingGC is beneficial? We can still make that the case (and
> > override initializeBackingThread()/shutdownBackingThread()) if you think
that
> is
> > better.
> 
> I think I worry that decoupling GC support and WebThread could make the code
> error-prone. We're going to have three combinations of GC support and
WebThread
> usage and every one's going to be implemented in a different file, which
> concerns me a bit.

The refactor out of GCSupportForWebThread from WebThreadSupportingGC in this
patch is with hopes to separate out the GC support from thread-ownership, while
still using the same code for doing GC (i.e. GCSupportForWebThread). The actual
code that does GC is still in a single place (GCSupportForWebThread), and the
worker code that manages GC is also in a single place (base WorkerThread class).
Making the normal workers create a WebThread, and making the base WorkerThread
class deal with GC (using GCSupportForWebThread) is also with the hope of making
sure we use the same code, and not end up with duplicated code.

Compositor workers here are different by design, and so manage their own GC.

> 
> How would it look to you/haraken if we also add detachThread() or something to
> WebThreadSupportingGC, which detaches the associated WebThread from
> WebThreadSupportingGC?

If we can add a mechanism in WebThreadSupportingGC where it doesn't own the
WebThread, then that should be sufficient, yes! So something like
detachThread(), or even a subclass of WebThreadSupportingGC that doesn't own the
WebThread (i.e. essentially GCSupportForWebThread), would both be sufficient for
this use.

[Takashi Toyoshima, 2015-08-13 18:48:34]

I don't know the historical background, so my following question could be a
newbie's one.

So when I started working around WorkerThread, I was a bit confused. The
WorkerThread was designed to represent a thread that hosts a worker script. But,
many abstraction layers were recently added so that CompositorWorkerThread that
didn't represent a thread can inherit the WorkerThread. Why should
CompositorWorkerThread inherit the WorkerThread? I feel CompositorWorkerManager
is better class to inherit the WorkerThread as a CompositorWorkerThread, and
current CompositorWorkerThread should be renamed to CompositorWorkerScript or
something. Maybe it would have duplicated code with WorkerThread. So script
related code could be factored out to a new WorkerScript class to be owned by
WorkerThread and CompositorWorkerScript.

I could not find out any caller of
CompositorWorkerMessagingProxy::createWorkerThread(). So, the caller may need
WorkerThread interface for some reasons?

[sadrul, 2015-08-13 20:40:39]

On 2015/08/13 18:48:34, Takashi Toyoshima (chromium) wrote:
> I don't know the historical background, so my following question could be a
> newbie's one.
> 
> So when I started working around WorkerThread, I was a bit confused. The
> WorkerThread was designed to represent a thread that hosts a worker script.
But,
> many abstraction layers were recently added so that CompositorWorkerThread
that
> didn't represent a thread can inherit the WorkerThread. Why should
> CompositorWorkerThread inherit the WorkerThread? I feel
CompositorWorkerManager
> is better class to inherit the WorkerThread

Right now, WorkerThread now owns the WorkerGlobalScope. For each
compositor-worker, we want to use the same thread, but separate execution
contexts (i.g. separate WorkerGlobalScopes). So having CompositorWorkerManager
inherit from WorkerThread would not be sufficient.

> as a CompositorWorkerThread, and
> current CompositorWorkerThread should be renamed to CompositorWorkerScript or
> something. Maybe it would have duplicated code with WorkerThread. So script
> related code could be factored out to a new WorkerScript class to be owned by
> WorkerThread and CompositorWorkerScript.

Yep. I think there is general agreement that name WorkerThread, as it is right
now, is a bit misleading. There was some attempt at renaming it to WorkerScript
(see: https://codereview.chromium.org/1115923002/). I think there is still
agreement that that would be an improvement, but that work as stalled a little
on some devtools related work that I haven't managed to get to yet,
unfortunately.

> 
> I could not find out any caller of
> CompositorWorkerMessagingProxy::createWorkerThread(). So, the caller may need
> WorkerThread interface for some reasons?

Not sure exactly what you mean, but it gets called through CompositorWorker
(is-a InProcessWorkerBase) --> WorkerMessagingProxy (-is-a
WorkerGlobalScopeProxy)::startWorkerGlobalScope ->
CompositorWorkerMessagingProxy (WorkerMessagingProxy)::createWorkerThread

[kinuko, 2015-08-31 11:58:45]

On 2015/08/11 04:45:29, sadrul wrote:
> On 2015/08/11 04:36:11, kinuko wrote:
> > On 2015/08/10 14:36:07, sadrul wrote:
> > > On 2015/08/10 14:23:37, haraken wrote:
> > > > On 2015/08/10 14:10:06, kinuko wrote:
> > > > > I think I prefer regular workers continue to have
WebThreadSupportingGC
> > > rather
> > > > > than decoupling it only for coding issues.  I wonder if we could add
one
> > > more
> > > > > create() method to WebThreadSupportingGC that takes an existing
thread,
> so
> > > > that
> > > > > we can create WebThreadSupportingGC for a given thread.  And the
> WebThread
> > > > > implementation for the compositor thread can be probably made in a way
> > that
> > > > > destructing the WebThread instance doesn't mean stopping the
underlying
> > > > thread. 
> > > > > Then we can consistently use WebThreadSupportingGC in all worker
thread
> > > code.
> > > > > Would something like that work / make sense?
> > > > 
> > > > Sounds like a plan!
> > > 
> > > I actually considered doing that at first. That way, there's little change
> to
> > be
> > > made in blink. However, WorkerThread deals with both the WebThread, and
its
> > > corresponding WebScheduler. So the Platform implementation will need to
> create
> > > proxies for both of these, and it becomes ugly pretty quickly.
> > > 
> > > Is there a particular reason you think having the workers continue to
create
> > > WebThreadSupportingGC is beneficial? We can still make that the case (and
> > > override initializeBackingThread()/shutdownBackingThread()) if you think
> that
> > is
> > > better.
> > 
> > I think I worry that decoupling GC support and WebThread could make the code
> > error-prone. We're going to have three combinations of GC support and
> WebThread
> > usage and every one's going to be implemented in a different file, which
> > concerns me a bit.
> 
> The refactor out of GCSupportForWebThread from WebThreadSupportingGC in this
> patch is with hopes to separate out the GC support from thread-ownership,
while
> still using the same code for doing GC (i.e. GCSupportForWebThread). The
actual
> code that does GC is still in a single place (GCSupportForWebThread), and the
> worker code that manages GC is also in a single place (base WorkerThread
class).
> Making the normal workers create a WebThread, and making the base WorkerThread
> class deal with GC (using GCSupportForWebThread) is also with the hope of
making
> sure we use the same code, and not end up with duplicated code.
> 
> Compositor workers here are different by design, and so manage their own GC.
> 
> > 
> > How would it look to you/haraken if we also add detachThread() or something
to
> > WebThreadSupportingGC, which detaches the associated WebThread from
> > WebThreadSupportingGC?
> 
> If we can add a mechanism in WebThreadSupportingGC where it doesn't own the
> WebThread, then that should be sufficient, yes! So something like
> detachThread(), or even a subclass of WebThreadSupportingGC that doesn't own
the
> WebThread (i.e. essentially GCSupportForWebThread), would both be sufficient
for
> this use.

I wanted to see how it'd look like, so I took a shot in this approach and
created https://codereview.chromium.org/1319363002/.