mus::GpuService: Support establish GpuChannel asynchronously.

mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async
modes. The renderer process will only establish gpu channel
synchronously. To satisfy chrome's requirements, we add the
async version EstablishGpuChannel on GpuService.

BUG=586390
Committed: https://crrev.com/83ec46db6602a30dc4a675d6736b3f93b2bcc8fe
Cr-Commit-Position: refs/heads/master@{#402581}

[Peng, 2016-06-22 16:16:58]

penghuang@chromium.org changed reviewers:
+ piman@chromium.org

[Peng, 2016-06-22 16:16:59]

Hi piman, PTAL. Thanks.

[piman, 2016-06-22 17:58:10]

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:23: runner->PostTask(FROM_HERE, callback);
nit: can we move the FROM_HERE to the caller? I.e. add a
tracked_objects::Location parameter (it's copyable) to this PostTask, and have
the caller code do base::Bind(&PostTast, FROM_HERE, runner, closure);
That way we can identify the source better in traces and other things.

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:56: return base::Singleton<GpuService,
Because initialization needs to be explicit, could we use a global pointer, that
would be initialized in ::Initialize instead of a Singleton, and DCHECK here
that the pointer is valid?

IOW:

static GpuService* g_gpu_service = nullptr;

void GpuService::Initialize(shell::Connector* connector) {
  DCHECK(!g_gpu_service);
  g_gpu_service = new GpuService();
  g_gpu_service->Init(connector);
}

GpuService* GpuService::GetInstance() {
  DCHECK(g_gpu_service);
  return g_gpu_service;
}


You may even be able to fold Init() into the constructor.

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:105: // so we have to run a nest run loop.
This seems bad.
Could we instead run the code below (other side of the if), and just make sure
the posted task is cancelled (e.g. return early if is_establishing_ is false
under the lock) ?

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:131: << "Channel encountered error while
establishing gpu channel.";
should you return nullptr here?

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
File components/mus/common/gpu_service.h (right):

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.h:76: base::Closure quite_run_loop_closure_;
nit: quite->quit

[Peng, 2016-06-22 19:25:39]

Patchset #3 (id:40001) has been deleted

[Peng, 2016-06-22 19:28:31]

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:23: runner->PostTask(FROM_HERE, callback);
On 2016/06/22 17:58:10, piman wrote:
> nit: can we move the FROM_HERE to the caller? I.e. add a
> tracked_objects::Location parameter (it's copyable) to this PostTask, and have
> the caller code do base::Bind(&PostTast, FROM_HERE, runner, closure);
> That way we can identify the source better in traces and other things.

Done.

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:56: return base::Singleton<GpuService,
On 2016/06/22 17:58:10, piman wrote:
> Because initialization needs to be explicit, could we use a global pointer,
that
> would be initialized in ::Initialize instead of a Singleton, and DCHECK here
> that the pointer is valid?
> 
> IOW:
> 
> static GpuService* g_gpu_service = nullptr;
> 
> void GpuService::Initialize(shell::Connector* connector) {
>   DCHECK(!g_gpu_service);
>   g_gpu_service = new GpuService();
>   g_gpu_service->Init(connector);
> }
> 
> GpuService* GpuService::GetInstance() {
>   DCHECK(g_gpu_service);
>   return g_gpu_service;
> }
> 
> 
> You may even be able to fold Init() into the constructor.

Done.

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:105: // so we have to run a nest run loop.
On 2016/06/22 17:58:10, piman wrote:
> This seems bad.
> Could we instead run the code below (other side of the if), and just make sure
> the posted task is cancelled (e.g. return early if is_establishing_ is false
> under the lock) ?

Done.

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.cc:131: << "Channel encountered error while
establishing gpu channel.";
On 2016/06/22 17:58:10, piman wrote:
> should you return nullptr here?

Done.

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
File components/mus/common/gpu_service.h (right):

https://codereview.chromium.org/2087333002/diff/20001/components/mus/common/g...
components/mus/common/gpu_service.h:76: base::Closure quite_run_loop_closure_;
On 2016/06/22 17:58:10, piman wrote:
> nit: quite->quit

Done.

[piman, 2016-06-22 21:33:52]

lgtm

[Peng, 2016-06-22 22:53:49]

penghuang@chromium.org changed reviewers:
+ sky@chromium.org

[Peng, 2016-06-22 22:53:50]

+sky for ash/* and mash/*
+jam for base/threading/*

Hi Scott & John, PTAL. Thanks.

[Peng, 2016-06-22 22:54:09]

The CQ bit was checked by penghuang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 22:55:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087333002/60001

[commit-bot: I haz the power, 2016-06-22 23:00:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-22 23:00:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peng, 2016-06-22 23:06:17]

penghuang@chromium.org changed reviewers:
+ jam@chromium.org

[Peng, 2016-06-22 23:06:18]

+jam for base/threading

Hi John, PTAL. Thanks.

[sky, 2016-06-22 23:49:21]

LGTM

[jam, 2016-06-22 23:55:51]

https://codereview.chromium.org/2087333002/diff/80001/base/threading/thread_r...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/2087333002/diff/80001/base/threading/thread_r...
base/threading/thread_restrictions.h:219: friend class mus::GpuService;         
         // http://crbug.com/620058
is this a bad merge? you need to remove this from mojo::SyncCallRestrictions now

[jam, 2016-06-22 23:57:25]

https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
components/mus/common/gpu_service.cc:114: if
(!gpu_service_.WaitForIncomingResponse()) {
i'm not sure I follow: why is this needed since the patch subject is to remove
sync waiting?

[Peng, 2016-06-23 14:15:57]

https://codereview.chromium.org/2087333002/diff/80001/base/threading/thread_r...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/2087333002/diff/80001/base/threading/thread_r...
base/threading/thread_restrictions.h:219: friend class mus::GpuService;         
         // http://crbug.com/620058
On 2016/06/22 23:55:50, jam wrote:
> is this a bad merge? you need to remove this from mojo::SyncCallRestrictions
now

Done.

https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
components/mus/common/gpu_service.cc:114: if
(!gpu_service_.WaitForIncomingResponse()) {
On 2016/06/22 23:57:25, jam wrote:
> i'm not sure I follow: why is this needed since the patch subject is to remove
> sync waiting?

GpuService supports EstablishGpuChannel in both sync and async mode. If an async
mojo EstablishGpuChannel has been issued but the response is not received, the
GpuService::EstablishGpuChannelSync() is called on the main thread, we have to
use WaitForIncomingResponse() to wait until the previous response is received.

[jam, 2016-06-23 15:27:23]

https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
components/mus/common/gpu_service.cc:114: if
(!gpu_service_.WaitForIncomingResponse()) {
On 2016/06/23 14:15:57, Peng wrote:
> On 2016/06/22 23:57:25, jam wrote:
> > i'm not sure I follow: why is this needed since the patch subject is to
remove
> > sync waiting?
> 
> GpuService supports EstablishGpuChannel in both sync and async mode. If an
async
> mojo EstablishGpuChannel has been issued but the response is not received, the
> GpuService::EstablishGpuChannelSync() is called on the main thread, we have to
> use WaitForIncomingResponse() to wait until the previous response is received.

Why do we need both async and sync modes? The cl description needs to explain
the purprose of this cl to make it clear to reviewers and anyone looking at the
change. Also, it should be linked to bugs related to this effort.

[Peng, 2016-06-23 15:49:35]

Description was changed from

==========
mus::GpuService: Support establish GpuChannel asynchronously.

BUG=None
==========

to

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
mode. The renderer process will only establish gpu channel asynchronously. To
satisfy chrome's requirements, we add a
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

[Peng, 2016-06-23 15:50:33]

Description was changed from

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
mode. The renderer process will only establish gpu channel asynchronously. To
satisfy chrome's requirements, we add a
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

to

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel asynchronously. To
satisfy chrome's requirements, we add a
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

[Peng, 2016-06-23 15:51:25]

Description was changed from

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel asynchronously. To
satisfy chrome's requirements, we add a
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

to

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel asynchronously.
To satisfy chrome's requirements, we add a
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

[Peng, 2016-06-23 15:56:39]

Description was changed from

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel asynchronously.
To satisfy chrome's requirements, we add a
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

to

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel
synchronously. To satisfy chrome's requirements, we add the
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

[Peng, 2016-06-23 15:56:51]

On 2016/06/23 15:27:23, jam wrote:
>
https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
> File components/mus/common/gpu_service.cc (right):
> 
>
https://codereview.chromium.org/2087333002/diff/80001/components/mus/common/g...
> components/mus/common/gpu_service.cc:114: if
> (!gpu_service_.WaitForIncomingResponse()) {
> On 2016/06/23 14:15:57, Peng wrote:
> > On 2016/06/22 23:57:25, jam wrote:
> > > i'm not sure I follow: why is this needed since the patch subject is to
> remove
> > > sync waiting?
> > 
> > GpuService supports EstablishGpuChannel in both sync and async mode. If an
> async
> > mojo EstablishGpuChannel has been issued but the response is not received,
the
> > GpuService::EstablishGpuChannelSync() is called on the main thread, we have
to
> > use WaitForIncomingResponse() to wait until the previous response is
received.
> 
> Why do we need both async and sync modes? The cl description needs to explain
> the purprose of this cl to make it clear to reviewers and anyone looking at
the
> change. Also, it should be linked to bugs related to this effort.

Just updated the description. PTAL.

Also see:
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
 BrowserGpuChannelHostFactory::EstablishGpuChannel
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
 BrowserGpuChannelHostFactory::EstablishGpuChannelSync 
https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?d...

RenderThreadImpl::EstablishGpuChannelSync

[jam, 2016-06-23 16:39:39]

https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
components/mus/common/gpu_service.cc:114:
base::ThreadRestrictions::ScopedAllowWait allow_wait;
why isn't this a mojo::SyncCallRestrictions::ScopedAllowSyncCall
allow_sync_call; and calling the sync GPU Service method instead of the async
one?

[jam, 2016-06-23 16:42:58]

https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
components/mus/common/gpu_service.cc:114:
base::ThreadRestrictions::ScopedAllowWait allow_wait;
On 2016/06/23 16:39:39, jam wrote:
> why isn't this a mojo::SyncCallRestrictions::ScopedAllowSyncCall
> allow_sync_call; and calling the sync GPU Service method instead of the async
> one?

This is undoing Yuzhu's change from yesterday that switched to sync calls
(https://codereview.chromium.org/2084993002/diff/40001/components/mus/common/g...).
see linked bug (we are removing WaitForIncomingResponse calls)

[Peng, 2016-06-23 17:07:37]

https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
File components/mus/common/gpu_service.cc (right):

https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
components/mus/common/gpu_service.cc:114:
base::ThreadRestrictions::ScopedAllowWait allow_wait;
On 2016/06/23 16:39:39, jam wrote:
> why isn't this a mojo::SyncCallRestrictions::ScopedAllowSyncCall
> allow_sync_call; and calling the sync GPU Service method instead of the async
> one?

Becasue maybe there is unfinished pending async method call, so we have to use
WaitForIncomingResponse() for that case. To make code simpler, if no pending
async method call, we just use async method call too.

If no WaitForIncomingResponse, probably we have to use RunLoop to start a nest
loop. But RunLoop is not recommended. :(

[jam, 2016-06-23 17:16:52]

On 2016/06/23 17:07:37, Peng wrote:
>
https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
> File components/mus/common/gpu_service.cc (right):
> 
>
https://codereview.chromium.org/2087333002/diff/140001/components/mus/common/...
> components/mus/common/gpu_service.cc:114:
> base::ThreadRestrictions::ScopedAllowWait allow_wait;
> On 2016/06/23 16:39:39, jam wrote:
> > why isn't this a mojo::SyncCallRestrictions::ScopedAllowSyncCall
> > allow_sync_call; and calling the sync GPU Service method instead of the
async
> > one?
> 
> Becasue maybe there is unfinished pending async method call, so we have to use
> WaitForIncomingResponse() for that case.

> To make code simpler, if no pending async method call, we just use async
method call too.

that can lead to deadlocks. if you're making sync calls, you need to use the
sync IPC functionality.

> 
> If no WaitForIncomingResponse, probably we have to use RunLoop to start a nest
> loop. But RunLoop is not recommended. :(

[jam, 2016-06-23 17:18:15]

also the new description says that browser may establish gpu channel in async
and sync modes. can you explain why it needs both? i.e. could we simplify this
problem by having it always establish it synchronously or always asynchronously?

[Peng, 2016-06-23 17:27:55]

On 2016/06/23 17:18:15, jam wrote:
> also the new description says that browser may establish gpu channel in async
> and sync modes. can you explain why it needs both? i.e. could we simplify this
> problem by having it always establish it synchronously or always
asynchronously?

I don't fully understand why current chrome browser process needs both modes.

Hi piman, Could you please give us some advice?

[piman, 2016-06-23 20:23:29]

On Thu, Jun 23, 2016 at 10:27 AM, <penghuang@chromium.org> wrote:

> On 2016/06/23 17:18:15, jam wrote:
> > also the new description says that browser may establish gpu channel in
> async
> > and sync modes. can you explain why it needs both? i.e. could we
> simplify this
> > problem by having it always establish it synchronously or always
> asynchronously?
>
> I don't fully understand why current chrome browser process needs both
> modes.
>
> Hi piman, Could you please give us some advice?
>

We've been progressively migrating things to use the async method in the
browser process, but not all calls have been converted yet.

I think at this point the Sync method is only used
for GpuProcessTransportFactory::SharedMainThreadContextProvider(), which is
only used for tests (they can probably be reworked) and
for GpuProcessTransportFactory::GetGLHelper(). I believe the places that
use GetGLHelper() already expect the channel to have been established (e.g.
to handle a copy request that was done with the compositor context), so we
may be able to assume that either the channel has been created (and take
that one), or that it is lost in which case establishing synchronously
won't help anyway (it would try to do things to resources that are gone).

Note, the renderer process needs a sync method, e.g. for WebGL (the API
from JavaScript allows the caller to synchronously create a context and
know whether it succeeds).


>
> https://codereview.chromium.org/2087333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2016-06-23 23:04:21]

On 2016/06/23 20:23:29, piman wrote:
> On Thu, Jun 23, 2016 at 10:27 AM, <mailto:penghuang@chromium.org> wrote:
> 
> > On 2016/06/23 17:18:15, jam wrote:
> > > also the new description says that browser may establish gpu channel in
> > async
> > > and sync modes. can you explain why it needs both? i.e. could we
> > simplify this
> > > problem by having it always establish it synchronously or always
> > asynchronously?
> >
> > I don't fully understand why current chrome browser process needs both
> > modes.
> >
> > Hi piman, Could you please give us some advice?
> >
> 
> We've been progressively migrating things to use the async method in the
> browser process, but not all calls have been converted yet.
> 
> I think at this point the Sync method is only used
> for GpuProcessTransportFactory::SharedMainThreadContextProvider(), which is
> only used for tests (they can probably be reworked) and
> for GpuProcessTransportFactory::GetGLHelper(). I believe the places that
> use GetGLHelper() already expect the channel to have been established (e.g.
> to handle a copy request that was done with the compositor context), so we
> may be able to assume that either the channel has been created (and take
> that one), or that it is lost in which case establishing synchronously
> won't help anyway (it would try to do things to resources that are gone).
> 
> Note, the renderer process needs a sync method, e.g. for WebGL (the API
> from JavaScript allows the caller to synchronously create a context and
> know whether it succeeds).

Thanks. So can we do the reworking of GpuProcessTransportFactory for tests and
then only end up with async from the browser? That would simplify the logic in
this change a lot and get rid of the need for blocking the browser.

> 
> 
> >
> > https://codereview.chromium.org/2087333002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-06-23 23:11:30]

On Thu, Jun 23, 2016 at 4:04 PM, <jam@chromium.org> wrote:

> On 2016/06/23 20:23:29, piman wrote:
>
> > On Thu, Jun 23, 2016 at 10:27 AM, <mailto:penghuang@chromium.org> wrote:
> >
> > > On 2016/06/23 17:18:15, jam wrote:
> > > > also the new description says that browser may establish gpu channel
> in
> > > async
> > > > and sync modes. can you explain why it needs both? i.e. could we
> > > simplify this
> > > > problem by having it always establish it synchronously or always
> > > asynchronously?
> > >
> > > I don't fully understand why current chrome browser process needs both
> > > modes.
> > >
> > > Hi piman, Could you please give us some advice?
> > >
> >
> > We've been progressively migrating things to use the async method in the
> > browser process, but not all calls have been converted yet.
> >
> > I think at this point the Sync method is only used
> > for GpuProcessTransportFactory::SharedMainThreadContextProvider(), which
> is
> > only used for tests (they can probably be reworked) and
> > for GpuProcessTransportFactory::GetGLHelper(). I believe the places that
> > use GetGLHelper() already expect the channel to have been established
> (e.g.
> > to handle a copy request that was done with the compositor context), so
> we
> > may be able to assume that either the channel has been created (and take
> > that one), or that it is lost in which case establishing synchronously
> > won't help anyway (it would try to do things to resources that are gone).
> >
> > Note, the renderer process needs a sync method, e.g. for WebGL (the API
> > from JavaScript allows the caller to synchronously create a context and
> > know whether it succeeds).
>
> Thanks. So can we do the reworking of GpuProcessTransportFactory for tests
> and
> then only end up with async from the browser? That would simplify the
> logic in
> this change a lot and get rid of the need for blocking the browser.
>

There's more than tests (e.g. copy requests, reflector etc.) as mentioned
above...

If someone wants to take it, I'm happy to review code, but I won't be able
to focus on this in the short term.

Antoine


> >
> >
> > >
> > > https://codereview.chromium.org/2087333002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2087333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2016-06-23 23:15:05]

On 2016/06/23 23:04:21, jam wrote:
> On 2016/06/23 20:23:29, piman wrote:
> > On Thu, Jun 23, 2016 at 10:27 AM, <mailto:penghuang@chromium.org> wrote:
> > 
> > > On 2016/06/23 17:18:15, jam wrote:
> > > > also the new description says that browser may establish gpu channel in
> > > async
> > > > and sync modes. can you explain why it needs both? i.e. could we
> > > simplify this
> > > > problem by having it always establish it synchronously or always
> > > asynchronously?
> > >
> > > I don't fully understand why current chrome browser process needs both
> > > modes.
> > >
> > > Hi piman, Could you please give us some advice?
> > >
> > 
> > We've been progressively migrating things to use the async method in the
> > browser process, but not all calls have been converted yet.
> > 
> > I think at this point the Sync method is only used
> > for GpuProcessTransportFactory::SharedMainThreadContextProvider(), which is
> > only used for tests (they can probably be reworked) and
> > for GpuProcessTransportFactory::GetGLHelper(). I believe the places that
> > use GetGLHelper() already expect the channel to have been established (e.g.
> > to handle a copy request that was done with the compositor context), so we
> > may be able to assume that either the channel has been created (and take
> > that one), or that it is lost in which case establishing synchronously
> > won't help anyway (it would try to do things to resources that are gone).
> > 
> > Note, the renderer process needs a sync method, e.g. for WebGL (the API
> > from JavaScript allows the caller to synchronously create a context and
> > know whether it succeeds).
> 
> Thanks. So can we do the reworking of GpuProcessTransportFactory for tests and
> then only end up with async from the browser? That would simplify the logic in
> this change a lot and get rid of the need for blocking the browser.

I am working on supporting HW rendering and would like to support it ASAP. I am
not
sure how much work is needed for GpuProcessTransportFactory reworking. So I
would like
to either land this CL without big change, or abandon this CL for now, and
always use the
sync method in GpuProcessTransportFactory. When the HW rendering is ready, I
will start
try to rework the GpuProcessTransportFactory to remove the sync method. WDYT?

[Peng, 2016-06-24 18:47:09]

Hi John, The CL has been updated. I removed the WaitForIncomingResponse(). PTAL.
Thanks.

[Peng, 2016-06-27 20:03:49]

On 2016/06/24 18:47:09, Peng wrote:
> Hi John, The CL has been updated. I removed the WaitForIncomingResponse().
PTAL.
> Thanks.

John, kindly ping :)

[jam, 2016-06-27 21:49:50]

On 2016/06/27 20:03:49, Peng wrote:
> On 2016/06/24 18:47:09, Peng wrote:
> > Hi John, The CL has been updated. I removed the WaitForIncomingResponse().
> PTAL.
> > Thanks.
> 
> John, kindly ping :)

There's still a 
base::ThreadRestrictions::ScopedAllowWait allow_wait;
that appears to be needed just when GpuService::EstablishGpuChannelSync() is
called outside the main thread. why is this needed? this can result in deadlock

[Peng, 2016-06-27 22:49:12]

On 2016/06/27 21:49:50, jam wrote:
> On 2016/06/27 20:03:49, Peng wrote:
> > On 2016/06/24 18:47:09, Peng wrote:
> > > Hi John, The CL has been updated. I removed the WaitForIncomingResponse().
> > PTAL.
> > > Thanks.
> > 
> > John, kindly ping :)
> 
> There's still a 
> base::ThreadRestrictions::ScopedAllowWait allow_wait;
> that appears to be needed just when GpuService::EstablishGpuChannelSync() is
> called outside the main thread. why is this needed? this can result in
deadlock

I see. so ScopedAllowWait should be removed. Done

[Peng, 2016-06-27 22:51:57]

Hi Antoine, the GpuService has been uploaded since your LGTM. Would you like
take a look again? Thanks.

[piman, 2016-06-27 23:08:44]

LGTM if jam@ is happy

[Peng, 2016-06-28 02:34:31]

On 2016/06/27 23:08:44, piman wrote:
> LGTM if jam@ is happy

Since I have followed jam@'s suggestion and removed the ScopedAllowWait, so we
don't need LGTM from jam@ anymore.

[Peng, 2016-06-28 02:46:02]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-06-28 02:46:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2087333002/#ps280001
(title: "Update")

[commit-bot: I haz the power, 2016-06-28 02:46:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 03:59:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 03:59:29]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-06-28 15:05:48]

On 2016/06/27 22:49:12, Peng wrote:
> On 2016/06/27 21:49:50, jam wrote:
> > On 2016/06/27 20:03:49, Peng wrote:
> > > On 2016/06/24 18:47:09, Peng wrote:
> > > > Hi John, The CL has been updated. I removed the
WaitForIncomingResponse().
> > > PTAL.
> > > > Thanks.
> > > 
> > > John, kindly ping :)
> > 
> > There's still a 
> > base::ThreadRestrictions::ScopedAllowWait allow_wait;
> > that appears to be needed just when GpuService::EstablishGpuChannelSync() is
> > called outside the main thread. why is this needed? this can result in
> deadlock
> 
> I see. so ScopedAllowWait should be removed. Done

What causes the deadlock is the establishing_condition_.Wait() line. The latest
patch removed the ScopedAllowWait, which just removes the dcheck. The deadlock
can still happen though (the scenario is if the thread which is waiting receives
a sync IPC from the other end of the message pipe).

I'm still trying to understand: why does GpuService::EstablishGpuChannelSync()
need to be called outside the main thread?

[jam, 2016-06-28 16:12:51]

On 2016/06/28 15:05:48, jam wrote:
> On 2016/06/27 22:49:12, Peng wrote:
> > On 2016/06/27 21:49:50, jam wrote:
> > > On 2016/06/27 20:03:49, Peng wrote:
> > > > On 2016/06/24 18:47:09, Peng wrote:
> > > > > Hi John, The CL has been updated. I removed the
> WaitForIncomingResponse().
> > > > PTAL.
> > > > > Thanks.
> > > > 
> > > > John, kindly ping :)
> > > 
> > > There's still a 
> > > base::ThreadRestrictions::ScopedAllowWait allow_wait;
> > > that appears to be needed just when GpuService::EstablishGpuChannelSync()
is
> > > called outside the main thread. why is this needed? this can result in
> > deadlock
> > 
> > I see. so ScopedAllowWait should be removed. Done
> 
> What causes the deadlock is the establishing_condition_.Wait() line. The
latest
> patch removed the ScopedAllowWait, which just removes the dcheck. The deadlock
> can still happen though (the scenario is if the thread which is waiting
receives
> a sync IPC from the other end of the message pipe).
> 
> I'm still trying to understand: why does GpuService::EstablishGpuChannelSync()
> need to be called outside the main thread?

(per chat, this isn't an issue since the gpu process doesn't make sync calls
back).
lgtm since the thread restriction exception is removed.

[Peng, 2016-06-28 20:58:41]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-06-28 20:58:41]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
sky@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2087333002/#ps300001
(title: "Fix the unittests")

[commit-bot: I haz the power, 2016-06-28 20:59:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 22:49:49]

Description was changed from

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel
synchronously. To satisfy chrome's requirements, we add the
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

to

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel
synchronously. To satisfy chrome's requirements, we add the
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

[commit-bot: I haz the power, 2016-06-28 22:49:51]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2016-06-28 22:51:37]

Description was changed from

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async 
modes. The renderer process will only establish gpu channel
synchronously. To satisfy chrome's requirements, we add the
async version EstablishGpuChannel on GpuService.


BUG=586390
==========

to

==========
mus::GpuService: Support establish GpuChannel asynchronously.

The mus::GpuService will be used by chrome browser process
and chrome renderer process in mus+ash. The current chrome
browser may establish Gpu channel in both sync and async
modes. The renderer process will only establish gpu channel
synchronously. To satisfy chrome's requirements, we add the
async version EstablishGpuChannel on GpuService.

BUG=586390

Committed: https://crrev.com/83ec46db6602a30dc4a675d6736b3f93b2bcc8fe
Cr-Commit-Position: refs/heads/master@{#402581}
==========

[commit-bot: I haz the power, 2016-06-28 22:51:38]

Patchset 15 (id:??) landed as
https://crrev.com/83ec46db6602a30dc4a675d6736b3f93b2bcc8fe
Cr-Commit-Position: refs/heads/master@{#402581}