Allow multiple ChildProcess objects in one process for the --single-process case, when we have bo...

Allow multiple ChildProcess objects in one process for the --single-process case, when we have both renderer and gpu.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87712

[apatrick_chromium, 2011-05-24 17:51:08]

LGTM

[Daniel Sievers (Google), 2011-05-24 18:24:30]

Won't this break things somewhere, because now retrieving
ChildProcess::current() will only work from the same thread that the object was
created on?

Also, do we really want multiple ChildProcess instances in the same process? It
looks to me like that would be creating multiple io threads (with the same
name).

I'm looking at it a bit more to see what might need to happen to runder render
and gpu thread in the same process, both having ipc channels.


On 2011/05/24 06:18:52, John Abd-El-Malek wrote:

[jam, 2011-05-24 19:10:29]

On 2011/05/24 18:24:30, Daniel Sievers wrote:
> Won't this break things somewhere, because now retrieving
> ChildProcess::current() will only work from the same thread that the object
was
> created on?

That is the intent.  These objects, like ChildThread, are not thread-safe.

> 
> Also, do we really want multiple ChildProcess instances in the same process?
It
> looks to me like that would be creating multiple io threads (with the same
> name).

What's wrong with having multiple IO threads?  It seems reasonable to have one
per SyncChannel.

> 
> I'm looking at it a bit more to see what might need to happen to runder render
> and gpu thread in the same process, both having ipc channels.
> 
> 
> On 2011/05/24 06:18:52, John Abd-El-Malek wrote:

[Daniel Sievers (Google), 2011-05-24 20:16:39]

Ok, having two ChildProcess instances and two io threads sounds fine then (one
for gpu one for renderer) in single process mode.

But I'm not convinced that ChildProcess objects shouldn't be able to have
thread-safe methods. In fact, how do we know there isn't even existing code that
calls ChildProcess::current() from different threads?

If we want to allow multiple ChildProcess instances in one process, then we
should rather deprecate ChildProcess::current() and require explicit calls to
RenderProcess::current() or GpuProcess::current() instead, which each have their
own static instance.

[jam, 2011-05-24 21:17:41]

On 2011/05/24 20:16:39, Daniel Sievers wrote:
> Ok, having two ChildProcess instances and two io threads sounds fine then (one
> for gpu one for renderer) in single process mode.
> 
> But I'm not convinced that ChildProcess objects shouldn't be able to have
> thread-safe methods. In fact, how do we know there isn't even existing code
that
> calls ChildProcess::current() from different threads?

If there is, it's trivial to catch it with the above since it'll now break.

> 
> If we want to allow multiple ChildProcess instances in one process, then we
> should rather deprecate ChildProcess::current() and require explicit calls to
> RenderProcess::current() or GpuProcess::current() instead, which each have
their
> own static instance.

there is some code that runs in both renderer+plugin (or renderer+worker).  that
code needs to be able to access ChildProcess directly

[jam, 2011-05-24 21:45:15]

On Tue, May 24, 2011 at 1:16 PM, <sievers@google.com> wrote:

>
> Ok, having two ChildProcess instances and two io threads sounds fine then
> (one
> for gpu one for renderer) in single process mode.
>
> But I'm not convinced that ChildProcess objects shouldn't be able to have
> thread-safe methods. In fact, how do we know there isn't even existing code
> that
> calls ChildProcess::current() from different threads?
>
> If we want to allow multiple ChildProcess instances in one process, then we
> should rather deprecate ChildProcess::current() and require explicit calls
> to
> RenderProcess::current() or GpuProcess::current() instead, which each have
> their
> own static instance.


btw, I'm fine with doing this.  however in order to do that, we'd need to
get rid of the existing ChildProcess::current calls.  that probably won't be
a huge task.


>
>
>
>
>
>
>
>
> http://codereview.chromium.org/6966030/
>

[Daniel Sievers (Google), 2011-05-25 20:33:41]

On 2011/05/24 21:45:15, John Abd-El-Malek wrote:
> On Tue, May 24, 2011 at 1:16 PM, <mailto:sievers@google.com> wrote:
> 
> >
> > Ok, having two ChildProcess instances and two io threads sounds fine then
> > (one
> > for gpu one for renderer) in single process mode.
> >
> > But I'm not convinced that ChildProcess objects shouldn't be able to have
> > thread-safe methods. In fact, how do we know there isn't even existing code
> > that
> > calls ChildProcess::current() from different threads?
> >
> > If we want to allow multiple ChildProcess instances in one process, then we
> > should rather deprecate ChildProcess::current() and require explicit calls
> > to
> > RenderProcess::current() or GpuProcess::current() instead, which each have
> > their
> > own static instance.
> 
> 
> btw, I'm fine with doing this.  however in order to do that, we'd need to
> get rid of the existing ChildProcess::current calls.  that probably won't be
> a huge task.
> 
> 


I agree that it is a bit ugly though in the way the static interface is
currently exposed. For example, it's odd that you cannot reach
ChildThread::current() from the wrong thread anymore (it returns null), while
you can still do ChildProcess::main_thread() from anywhere.

So maybe if we end up touching all the code that calls ChildProcess::current(),
it might be worth changing the class to only expose static interfaces for
threadsafe and useful functions (such as posting a task to some thread), rather
than exposing the *whole* class interface as a static member.


> >
> >
> >
> >
> >
> >
> >
> >
> > http://codereview.chromium.org/6966030/
> >

[jam, 2011-05-25 20:59:51]

On Wed, May 25, 2011 at 1:33 PM, <sievers@google.com> wrote:

> On 2011/05/24 21:45:15, John Abd-El-Malek wrote:
>
>  On Tue, May 24, 2011 at 1:16 PM, <mailto:sievers@google.com> wrote:
>>
>
>  >
>> > Ok, having two ChildProcess instances and two io threads sounds fine
>> then
>> > (one
>> > for gpu one for renderer) in single process mode.
>> >
>> > But I'm not convinced that ChildProcess objects shouldn't be able to
>> have
>> > thread-safe methods. In fact, how do we know there isn't even existing
>> code
>> > that
>> > calls ChildProcess::current() from different threads?
>> >
>> > If we want to allow multiple ChildProcess instances in one process, then
>> we
>> > should rather deprecate ChildProcess::current() and require explicit
>> calls
>> > to
>> > RenderProcess::current() or GpuProcess::current() instead, which each
>> have
>> > their
>> > own static instance.
>>
>
>
>  btw, I'm fine with doing this.  however in order to do that, we'd need to
>> get rid of the existing ChildProcess::current calls.  that probably won't
>> be
>> a huge task.
>>
>
>
>
>
> I agree that it is a bit ugly though in the way the static interface is
> currently exposed. For example, it's odd that you cannot reach
> ChildThread::current() from the wrong thread anymore (it returns null),
> while
> you can still do ChildProcess::main_thread() from anywhere.
>

Once ChildThread::current uses the TLS, then no need to expose main_thread()
anymore from ChildProcess.


>
> So maybe if we end up touching all the code that calls
> ChildProcess::current(),
> it might be worth changing the class to only expose static interfaces for
> threadsafe and useful functions (such as posting a task to some thread),
> rather
> than exposing the *whole* class interface as a static member.


This isn't a pattern that we usually do, so I'm wary of doing that.

>
>
>
>  >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > http://codereview.chromium.org/6966030/
>> >
>>
>
>
>
> http://codereview.chromium.org/6966030/
>