cc: Implement shared worker contexts. (v1)

cc: Implement shared worker contexts.

This moves the responsibility to call BindToCurrentThread/SetupLock out
of cc::OutputSurface and to the maintainer of the (possibly) shared
context.

Worker contexts are now destroyed on the thread they are bound to. The
context provider lock needs to be acquired to safely use a worker
context on any thread, including the thread that the context is bound
to. To safely use a worker context on a different thread it's also
required to check if the context has been destroyed.

BUG=523411
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[danakj, 2015-08-25 23:06:21]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-08-25 23:06:21]

That LGTM

[danakj, 2015-08-25 23:07:19]

Will you need to do this in any of the cc pixel test harness code too maybe?

On Tue, Aug 25, 2015 at 4:06 PM, <danakj@chromium.org> wrote:

> That LGTM
>
> https://codereview.chromium.org/1317743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2015-08-26 01:24:24]

On 2015/08/25 at 23:07:19, danakj wrote:
> Will you need to do this in any of the cc pixel test harness code too maybe?

Yea, I updated the test code in latest patch.

[reveman, 2015-08-26 01:24:28]

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

[commit-bot: I haz the power, 2015-08-26 01:24:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/20001

[commit-bot: I haz the power, 2015-08-26 01:48:34]

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

[commit-bot: I haz the power, 2015-08-26 01:48:34]

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

[reveman, 2015-08-26 15:15:04]

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

[commit-bot: I haz the power, 2015-08-26 15:16:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/40001

[commit-bot: I haz the power, 2015-08-26 15:35:36]

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

[commit-bot: I haz the power, 2015-08-26 15:35:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[reveman, 2015-08-26 18:32:59]

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

[commit-bot: I haz the power, 2015-08-26 18:34:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/40001

[danakj, 2015-08-26 18:48:50]

https://codereview.chromium.org/1317743002/diff/40001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool_unittest.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool_unittest.cc:145:
base::ThreadTaskRunnerHandle::Get()
Did git cl format do this? If so can you please file a bug? That can't be right.

https://codereview.chromium.org/1317743002/diff/40001/cc/test/test_context_pr...
File cc/test/test_context_provider.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/cc/test/test_context_pr...
cc/test/test_context_provider.cc:28:
Create(TestWebGraphicsContext3D::Create().Pass());
you don't need .Pass() for a return value

https://codereview.chromium.org/1317743002/diff/40001/cc/test/test_context_pr...
cc/test/test_context_provider.cc:31: worker_context_provider->bound_ = true;
Can you explain this with a comment? Prod code in fact does call
BindToCurrentThread for the worker context..

https://codereview.chromium.org/1317743002/diff/40001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:271:
shared_worker_context_provider_->DetachFromThread();
Please comment and explain

https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1031:
worker_context_provider->DetachFromThread();
ditto

[commit-bot: I haz the power, 2015-08-26 19:07:28]

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

[commit-bot: I haz the power, 2015-08-26 19:07:29]

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

[jbauman, 2015-08-26 20:59:24]

jbauman@chromium.org changed reviewers:
+ jbauman@chromium.org

[jbauman, 2015-08-26 20:59:25]

https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1026:
!worker_context_provider->BindToCurrentThread()) {
I think we want this to happen on the compositor thread, as that's where all the
callbacks will be executed.

[reveman, 2015-08-26 22:02:11]

This is still WIP as I'm not sure it passes all tests yet but feel free to take
a look at current changes and let me know if they don't make sense.

https://codereview.chromium.org/1317743002/diff/40001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool_unittest.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool_unittest.cc:145:
base::ThreadTaskRunnerHandle::Get()
On 2015/08/26 at 18:48:50, danakj wrote:
> Did git cl format do this? If so can you please file a bug? That can't be
right.

crbug.com/525222

https://codereview.chromium.org/1317743002/diff/40001/cc/test/test_context_pr...
File cc/test/test_context_provider.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/cc/test/test_context_pr...
cc/test/test_context_provider.cc:28:
Create(TestWebGraphicsContext3D::Create().Pass());
On 2015/08/26 at 18:48:50, danakj wrote:
> you don't need .Pass() for a return value

Done.

https://codereview.chromium.org/1317743002/diff/40001/cc/test/test_context_pr...
cc/test/test_context_provider.cc:31: worker_context_provider->bound_ = true;
On 2015/08/26 at 18:48:50, danakj wrote:
> Can you explain this with a comment? Prod code in fact does call
BindToCurrentThread for the worker context..

Done.

https://codereview.chromium.org/1317743002/diff/40001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:271:
shared_worker_context_provider_->DetachFromThread();
On 2015/08/26 at 18:48:50, danakj wrote:
> Please comment and explain

Done.

https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1026:
!worker_context_provider->BindToCurrentThread()) {
On 2015/08/26 at 20:59:25, jbauman wrote:
> I think we want this to happen on the compositor thread, as that's where all
the callbacks will be executed.

The idea is that worker context callbacks are instead handled on the current
thread as it can be shared between compositors and other parts of chromium.
Mainly because that easier to setup as the context provider is created here and
not on the compositor thread but also to make it easier to reason about usage
from multiple threads. The context is currently only used on worker threads by
cc/ but could also be used by the media thread in the future and in that case
handling callbacks on the compositor thread seems weird.

https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1031:
worker_context_provider->DetachFromThread();
On 2015/08/26 at 18:48:50, danakj wrote:
> ditto

Done.

[jbauman, 2015-08-26 22:33:02]

On 2015/08/26 22:02:11, reveman wrote:
> 
>
https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
> content/renderer/render_widget.cc:1026:
> !worker_context_provider->BindToCurrentThread()) {
> On 2015/08/26 at 20:59:25, jbauman wrote:
> > I think we want this to happen on the compositor thread, as that's where all
> the callbacks will be executed.
> 
> The idea is that worker context callbacks are instead handled on the current
> thread as it can be shared between compositors and other parts of chromium.
> Mainly because that easier to setup as the context provider is created here
and
> not on the compositor thread but also to make it easier to reason about usage
> from multiple threads. The context is currently only used on worker threads by
> cc/ but could also be used by the media thread in the future and in that case
> handling callbacks on the compositor thread seems weird.


But there isn't a "current thread" in all cases. Callbacks are driven by the
IPCs from the GPU process, and are always routed to whatever thread the context
provider is first made current on. Raster worker threads don't even have message
loops, so there's no way that they could run callbacks.

Handling it like this causes problems because there's no way to detect if we
lose the worker context but not the compositor context.

[reveman, 2015-08-26 22:49:40]

On 2015/08/26 at 22:33:02, jbauman wrote:
> On 2015/08/26 22:02:11, reveman wrote:
> > 
> >
https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
> > File content/renderer/render_widget.cc (right):
> > 
> >
https://codereview.chromium.org/1317743002/diff/40001/content/renderer/render...
> > content/renderer/render_widget.cc:1026:
> > !worker_context_provider->BindToCurrentThread()) {
> > On 2015/08/26 at 20:59:25, jbauman wrote:
> > > I think we want this to happen on the compositor thread, as that's where
all
> > the callbacks will be executed.
> > 
> > The idea is that worker context callbacks are instead handled on the current
> > thread as it can be shared between compositors and other parts of chromium.
> > Mainly because that easier to setup as the context provider is created here
and
> > not on the compositor thread but also to make it easier to reason about
usage
> > from multiple threads. The context is currently only used on worker threads
by
> > cc/ but could also be used by the media thread in the future and in that
case
> > handling callbacks on the compositor thread seems weird.
> 
> 
> But there isn't a "current thread" in all cases. Callbacks are driven by the
IPCs from the GPU process, and are always routed to whatever thread the context
provider is first made current on. Raster worker threads don't even have message
loops, so there's no way that they could run callbacks.
> 
> Handling it like this causes problems because there's no way to detect if we
lose the worker context but not the compositor context.

By "current thread" I meant the thread that is current when we first bind the
context. That's the main thread in the renderer and I guess the UI thread in the
case of the browser.

[reveman, 2015-08-27 17:27:34]

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

[commit-bot: I haz the power, 2015-08-27 17:28:32]

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

[danakj, 2015-08-27 18:24:01]

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
cc/test/test_context_provider.h:28: // Creates a worker context provider that
doesn't need to be bound to a
Hm.. but this doesn't match the real context. Diverging behaviour between the
test thing and the real thing reduces our test coverage. Can we avoid that?

[reveman, 2015-08-27 18:58:53]

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
cc/test/test_context_provider.h:28: // Creates a worker context provider that
doesn't need to be bound to a
On 2015/08/27 at 18:24:01, danakj wrote:
> Hm.. but this doesn't match the real context. Diverging behaviour between the
test thing and the real thing reduces our test coverage. Can we avoid that?

Create(); BindToCurrentThread(); SetupLock(); DetachFromThread() match what we
do for real contexts. This is just an utility function to avoid doing that in a
bunch of places for tests. I can of course make the implementation of this
function explicitly do all that but with this being part of the
TestContextProvider impl I figured that was unnecessary as the end result is the
same.

[danakj, 2015-08-27 20:16:40]

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
cc/test/test_context_provider.h:28: // Creates a worker context provider that
doesn't need to be bound to a
On 2015/08/27 18:58:52, reveman wrote:
> On 2015/08/27 at 18:24:01, danakj wrote:
> > Hm.. but this doesn't match the real context. Diverging behaviour between
the
> test thing and the real thing reduces our test coverage. Can we avoid that?
> 
> Create(); BindToCurrentThread(); SetupLock(); DetachFromThread() match what we
> do for real contexts. This is just an utility function to avoid doing that in
a
> bunch of places for tests. I can of course make the implementation of this
> function explicitly do all that but with this being part of the
> TestContextProvider impl I figured that was unnecessary as the end result is
the
> same.

The thing is we're calling this all the places that take a worker context, so
all our unit tests - including the ones that test that we do call stuff like
BindToCurrentThread - are getting a context with a different state than what a
real one would have.

I'd prefer if some set of tests would benefit from this type of thing we make it
a helper method in those tests' base class or something, I think. Is there many
tests this helps?

[reveman, 2015-08-27 20:33:07]

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
cc/test/test_context_provider.h:28: // Creates a worker context provider that
doesn't need to be bound to a
On 2015/08/27 at 20:16:40, danakj wrote:
> On 2015/08/27 18:58:52, reveman wrote:
> > On 2015/08/27 at 18:24:01, danakj wrote:
> > > Hm.. but this doesn't match the real context. Diverging behaviour between
the
> > test thing and the real thing reduces our test coverage. Can we avoid that?
> > 
> > Create(); BindToCurrentThread(); SetupLock(); DetachFromThread() match what
we
> > do for real contexts. This is just an utility function to avoid doing that
in a
> > bunch of places for tests. I can of course make the implementation of this
> > function explicitly do all that but with this being part of the
> > TestContextProvider impl I figured that was unnecessary as the end result is
the
> > same.
> 
> The thing is we're calling this all the places that take a worker context, so
all our unit tests - including the ones that test that we do call stuff like
BindToCurrentThread - are getting a context with a different state than what a
real one would have.
> 
> I'd prefer if some set of tests would benefit from this type of thing we make
it a helper method in those tests' base class or something, I think. Is there
many tests this helps?

I'm not sure I follow. This is needed in all cases where I call it in this
patch. cc::OutputSurface will no longer call BindToCurrentThread() on the worker
context and the worker context passed to the output surface will instead have to
be bound and setup in advance. CreateWorker() does this setup so we don't have
to do it in each case where we need a worker context for tests. I don't mind
moving this helper code somewhere else. e.g. static helper function as part of
FakeOutputSurface. Just let me know what you prefer and I'll move it.

[danakj, 2015-08-27 20:37:05]

On 2015/08/27 20:33:07, reveman wrote:
>
https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
> File cc/test/test_context_provider.h (right):
> 
>
https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
> cc/test/test_context_provider.h:28: // Creates a worker context provider that
> doesn't need to be bound to a
> On 2015/08/27 at 20:16:40, danakj wrote:
> > On 2015/08/27 18:58:52, reveman wrote:
> > > On 2015/08/27 at 18:24:01, danakj wrote:
> > > > Hm.. but this doesn't match the real context. Diverging behaviour
between
> the
> > > test thing and the real thing reduces our test coverage. Can we avoid
that?
> > > 
> > > Create(); BindToCurrentThread(); SetupLock(); DetachFromThread() match
what
> we
> > > do for real contexts. This is just an utility function to avoid doing that
> in a
> > > bunch of places for tests. I can of course make the implementation of this
> > > function explicitly do all that but with this being part of the
> > > TestContextProvider impl I figured that was unnecessary as the end result
is
> the
> > > same.
> > 
> > The thing is we're calling this all the places that take a worker context,
so
> all our unit tests - including the ones that test that we do call stuff like
> BindToCurrentThread - are getting a context with a different state than what a
> real one would have.
> > 
> > I'd prefer if some set of tests would benefit from this type of thing we
make
> it a helper method in those tests' base class or something, I think. Is there
> many tests this helps?
> 
> I'm not sure I follow. This is needed in all cases where I call it in this
> patch. cc::OutputSurface will no longer call BindToCurrentThread() on the
worker
> context and the worker context passed to the output surface will instead have
to
> be bound and setup in advance. CreateWorker() does this setup so we don't have
> to do it in each case where we need a worker context for tests. I don't mind
> moving this helper code somewhere else. e.g. static helper function as part of
> FakeOutputSurface. Just let me know what you prefer and I'll move it.

But most tests don't use the worker context. This is where I don't want us to
use it especially:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...
As that hits every layer tree test.

So making it implicit in using FakeOutputSurface isn't great. Like if you stop
doing this how many tests actually fail? It seems like it would only be tests
that aren't layer tree tests and also use a worker context. So, a few pixel
tests? And they could just do this setup for those tests?

[commit-bot: I haz the power, 2015-08-27 20:56:09]

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

[commit-bot: I haz the power, 2015-08-27 20:56:09]

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

[reveman, 2015-08-27 21:04:20]

On 2015/08/27 at 20:37:05, danakj wrote:
> On 2015/08/27 20:33:07, reveman wrote:
> >
https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
> > File cc/test/test_context_provider.h (right):
> > 
> >
https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
> > cc/test/test_context_provider.h:28: // Creates a worker context provider
that
> > doesn't need to be bound to a
> > On 2015/08/27 at 20:16:40, danakj wrote:
> > > On 2015/08/27 18:58:52, reveman wrote:
> > > > On 2015/08/27 at 18:24:01, danakj wrote:
> > > > > Hm.. but this doesn't match the real context. Diverging behaviour
between
> > the
> > > > test thing and the real thing reduces our test coverage. Can we avoid
that?
> > > > 
> > > > Create(); BindToCurrentThread(); SetupLock(); DetachFromThread() match
what
> > we
> > > > do for real contexts. This is just an utility function to avoid doing
that
> > in a
> > > > bunch of places for tests. I can of course make the implementation of
this
> > > > function explicitly do all that but with this being part of the
> > > > TestContextProvider impl I figured that was unnecessary as the end
result is
> > the
> > > > same.
> > > 
> > > The thing is we're calling this all the places that take a worker context,
so
> > all our unit tests - including the ones that test that we do call stuff like
> > BindToCurrentThread - are getting a context with a different state than what
a
> > real one would have.
> > > 
> > > I'd prefer if some set of tests would benefit from this type of thing we
make
> > it a helper method in those tests' base class or something, I think. Is
there
> > many tests this helps?
> > 
> > I'm not sure I follow. This is needed in all cases where I call it in this
> > patch. cc::OutputSurface will no longer call BindToCurrentThread() on the
worker
> > context and the worker context passed to the output surface will instead
have to
> > be bound and setup in advance. CreateWorker() does this setup so we don't
have
> > to do it in each case where we need a worker context for tests. I don't mind
> > moving this helper code somewhere else. e.g. static helper function as part
of
> > FakeOutputSurface. Just let me know what you prefer and I'll move it.
> 
> But most tests don't use the worker context. This is where I don't want us to
use it especially:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...
As that hits every layer tree test.
> 
> So making it implicit in using FakeOutputSurface isn't great. Like if you stop
doing this how many tests actually fail? It seems like it would only be tests
that aren't layer tree tests and also use a worker context. So, a few pixel
tests? And they could just do this setup for those tests?

I don't think we should be passing a worker context to cc::OutputSurface that is
not setup as worker context is expected to be setup but I could try to reduce
the number of FakeOutputSurface instances we create for tests that have a worker
context and we can see where that takes us. I'll do that in a separate patch
though.

[danakj, 2015-08-27 21:06:40]

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/60001/cc/test/test_context_pr...
cc/test/test_context_provider.h:28: // Creates a worker context provider that
doesn't need to be bound to a
On 2015/08/27 20:33:07, reveman wrote:
> On 2015/08/27 at 20:16:40, danakj wrote:
> > On 2015/08/27 18:58:52, reveman wrote:
> > > On 2015/08/27 at 18:24:01, danakj wrote:
> > > > Hm.. but this doesn't match the real context. Diverging behaviour
between
> the
> > > test thing and the real thing reduces our test coverage. Can we avoid
that?
> > > 
> > > Create(); BindToCurrentThread(); SetupLock(); DetachFromThread() match
what
> we
> > > do for real contexts. This is just an utility function to avoid doing that
> in a
> > > bunch of places for tests. I can of course make the implementation of this
> > > function explicitly do all that but with this being part of the
> > > TestContextProvider impl I figured that was unnecessary as the end result
is
> the
> > > same.
> > 
> > The thing is we're calling this all the places that take a worker context,
so
> all our unit tests - including the ones that test that we do call stuff like
> BindToCurrentThread - are getting a context with a different state than what a
> real one would have.
> > 
> > I'd prefer if some set of tests would benefit from this type of thing we
make
> it a helper method in those tests' base class or something, I think. Is there
> many tests this helps?
> 
> I'm not sure I follow. This is needed in all cases where I call it in this
> patch. cc::OutputSurface will no longer call BindToCurrentThread() on the
worker
> context and the worker context passed to the output surface will instead have
to
> be bound and setup in advance. CreateWorker() does this setup so we don't have
> to do it in each case where we need a worker context for tests. I don't mind
> moving this helper code somewhere else. e.g. static helper function as part of
> FakeOutputSurface. Just let me know what you prefer and I'll move it.

Oh.. ok I think I see what you're saying.

These methods are done by the embedder not by cc. So this is acting like how the
embedder would behave.

It's confusing that both the embedder and cc do BindToCurrentThread on the
output surface, sorry.

I think this LGTM. Thanks for talking through it.

[reveman, 2015-08-27 21:10:14]

reveman@chromium.org changed reviewers:
+ avi@chromium.org

[reveman, 2015-08-27 21:10:15]

jbauman, does this change make sense in general and does latest patch look ok?

+avi for content/

[Avi (use Gerrit), 2015-08-27 21:20:40]

content lgtm

[jbauman, 2015-08-27 22:26:41]

lgtm, I guess this works.

[reveman, 2015-08-27 22:55:36]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-08-27 22:55:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/60001

[commit-bot: I haz the power, 2015-08-28 02:32:54]

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

[commit-bot: I haz the power, 2015-08-28 02:32:55]

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

[reveman, 2015-09-01 16:05:25]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-09-01 16:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/60001

[reveman, 2015-09-01 18:40:58]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-09-01 18:40:58]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
avi@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/1317743002/#ps80001
(title: "webview fix")

[commit-bot: I haz the power, 2015-09-01 18:41:21]

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

[commit-bot: I haz the power, 2015-09-01 19:48:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-01 19:49:07]

Patchset 5 (id:??) landed as
https://crrev.com/10b73d7c140bc67ddeeaf716eb31df0977fe2497
Cr-Commit-Position: refs/heads/master@{#346712}

[jbauman, 2015-09-01 21:19:07]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1317533003/ by jbauman@chromium.org.

The reason for reverting is: Likely causes crash on Mac:
https://build.chromium.org/p/chromium.gpu/builders/Mac%20Release%20%28Intel%2...

Going to see if reverting helps.
BUG=527186.

[reveman, 2015-09-02 04:17:48]

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

[commit-bot: I haz the power, 2015-09-02 04:17:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/120001

[commit-bot: I haz the power, 2015-09-02 04:43:56]

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

[commit-bot: I haz the power, 2015-09-02 04:43:57]

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

[reveman, 2015-09-02 16:39:04]

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

[commit-bot: I haz the power, 2015-09-02 16:39:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/140001

[commit-bot: I haz the power, 2015-09-02 17:55:30]

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

[commit-bot: I haz the power, 2015-09-02 17:55:31]

Dry run: This issue passed the CQ dry run.

[reveman, 2015-09-02 18:11:25]

reveman@chromium.org changed reviewers:
+ kbr@chromium.org, piman@chromium.org

[reveman, 2015-09-02 18:11:26]

I think the latest patch should fix the issue that caused this to be reverted.
It makes it safe to destroy ContextProviderCommandBuffer on any thread. See
DestroyContext3d related code in context_provider_command_buffer.cc.

+piman
+kbr

I'm going to try to re-land this unless someone has any objections to the
changes in the latest patch.

[danakj, 2015-09-02 18:19:28]

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:82: if
(context3d_task_runner_.get() && base::ThreadTaskRunnerHandle::IsSet() &&
you don't need .get() when checking boolness

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:83:
base::ThreadTaskRunnerHandle::Get() != context3d_task_runner_.get()) {
I dislike the pattern of "if not on the thread, post task" and would very much
prefer to just always post task.

But why do we need to do this in the ContextProvider? It feels a bit sketchy?

How about we don't keep a reference on the context provider on the worker
thread, and hold the reference on the compositor thread instead. That should
provide a pretty clear lifetime? And not require changing this class which is
used all over the place?

[piman, 2015-09-02 18:26:08]

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:85: FROM_HERE,
base::Bind(&DestroyContext3d, base::Passed(&context3d_)));
The potential issue is that it may just mask the problem and possibly leak the
context, if the target thread was destroyed.

Agreed that ContextProviderCommandBuffer being refcounted may be misleading - I
wonder if we still need that.

[danakj, 2015-09-02 18:32:28]

On Wed, Sep 2, 2015 at 11:26 AM, <piman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
> File content/common/gpu/client/context_provider_command_buffer.cc
> (right):
>
>
>
https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
> content/common/gpu/client/context_provider_command_buffer.cc:85:
> FROM_HERE, base::Bind(&DestroyContext3d, base::Passed(&context3d_)));
> The potential issue is that it may just mask the problem and possibly
> leak the context, if the target thread was destroyed.
>
> Agreed that ContextProviderCommandBuffer being refcounted may be
> misleading - I wonder if we still need that.
>

Maybe we don't without a threaded browser compositor. IIRC I think it was
largely because we had 2 contexts, one that was owned by the main thread
and one owned by the compositor thread. So 2 lost context callbacks. But
each place wanted to delete both contexts when they received their callback.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2015-09-02 18:53:09]

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:82: if
(context3d_task_runner_.get() && base::ThreadTaskRunnerHandle::IsSet() &&
On 2015/09/02 at 18:19:28, danakj wrote:
> you don't need .get() when checking boolness

Done.

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:83:
base::ThreadTaskRunnerHandle::Get() != context3d_task_runner_.get()) {
On 2015/09/02 at 18:19:27, danakj wrote:
> I dislike the pattern of "if not on the thread, post task" and would very much
prefer to just always post task.

I agree and that's what I initially did but it breaks a huge amount of tests
that use this on one thread and expects synchronous tear down.

> 
> But why do we need to do this in the ContextProvider? It feels a bit sketchy?

We need it because ContextProvider is thread-safe-ref-counted and
ContextProviderCommandBuffer::context3d_ is not but expects to be created and
destroyed on the same thread.

> 
> How about we don't keep a reference on the context provider on the worker
thread, and hold the reference on the compositor thread instead. That should
provide a pretty clear lifetime? And not require changing this class which is
used all over the place?

We don't keep a reference on any of the worker threads afaik. The problem is
that we keep a reference on the compositor thread and it ends up being destroyed
on that thread while it was initialized on the main thread. As long as the
ContextProvider is ref-counted, it will be hard to provide any guarantees for
where and when it's destroyed.

I think the alternative is to remove ref-counting from ContentProvider but I
don't know how hard that would be or if it's even possible. I'm assuming it's
ref-counted for a good reason.

[danakj, 2015-09-02 21:15:36]

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:83:
base::ThreadTaskRunnerHandle::Get() != context3d_task_runner_.get()) {
On 2015/09/02 18:53:08, reveman wrote:
> On 2015/09/02 at 18:19:27, danakj wrote:
> > I dislike the pattern of "if not on the thread, post task" and would very
much
> prefer to just always post task.
> 
> I agree and that's what I initially did but it breaks a huge amount of tests
> that use this on one thread and expects synchronous tear down.
> > 
> > But why do we need to do this in the ContextProvider? It feels a bit
sketchy?
> 
> We need it because ContextProvider is thread-safe-ref-counted and
> ContextProviderCommandBuffer::context3d_ is not but expects to be created and
> destroyed on the same thread.
> 
> > 
> > How about we don't keep a reference on the context provider on the worker
> thread, and hold the reference on the compositor thread instead. That should
> provide a pretty clear lifetime? And not require changing this class which is
> used all over the place?
> 
> We don't keep a reference on any of the worker threads afaik. The problem is
> that we keep a reference on the compositor thread and it ends up being
destroyed
> on that thread while it was initialized on the main thread. As long as the
> ContextProvider is ref-counted, it will be hard to provide any guarantees for
> where and when it's destroyed.
> 
> I think the alternative is to remove ref-counting from ContentProvider but I
> don't know how hard that would be or if it's even possible. I'm assuming it's
> ref-counted for a good reason.

I guess this all makes me kinda sad then. Maybe we could make this not
refcounted. Or solve all of this some other way. Idk. Not that I have solid
options here without spending the time to try them out, so sorry :( 
https://codereview.chromium.org/1311093002/ sure was a simpler fix that didn't
involve posting deletes. :/ Did it have other downsides?

[reveman, 2015-09-02 21:32:35]

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1317743002/diff/140001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:83:
base::ThreadTaskRunnerHandle::Get() != context3d_task_runner_.get()) {
On 2015/09/02 at 21:15:36, danakj wrote:
> On 2015/09/02 18:53:08, reveman wrote:
> > On 2015/09/02 at 18:19:27, danakj wrote:
> > > I dislike the pattern of "if not on the thread, post task" and would very
much
> > prefer to just always post task.
> > 
> > I agree and that's what I initially did but it breaks a huge amount of tests
> > that use this on one thread and expects synchronous tear down.
> > > 
> > > But why do we need to do this in the ContextProvider? It feels a bit
sketchy?
> > 
> > We need it because ContextProvider is thread-safe-ref-counted and
> > ContextProviderCommandBuffer::context3d_ is not but expects to be created
and
> > destroyed on the same thread.
> > 
> > > 
> > > How about we don't keep a reference on the context provider on the worker
> > thread, and hold the reference on the compositor thread instead. That should
> > provide a pretty clear lifetime? And not require changing this class which
is
> > used all over the place?
> > 
> > We don't keep a reference on any of the worker threads afaik. The problem is
> > that we keep a reference on the compositor thread and it ends up being
destroyed
> > on that thread while it was initialized on the main thread. As long as the
> > ContextProvider is ref-counted, it will be hard to provide any guarantees
for
> > where and when it's destroyed.
> > 
> > I think the alternative is to remove ref-counting from ContentProvider but I
> > don't know how hard that would be or if it's even possible. I'm assuming
it's
> > ref-counted for a good reason.
> 
> I guess this all makes me kinda sad then. Maybe we could make this not
refcounted. Or solve all of this some other way. Idk. Not that I have solid
options here without spending the time to try them out, so sorry :( 
https://codereview.chromium.org/1311093002/ sure was a simpler fix that didn't
involve posting deletes. :/ Did it have other downsides?

I'm looking at making this not refcounted. In a separate patch but I'll update
this issue with the status. That other patch doesn't solve all issues. Worker
contexts might still be deleted on the wrong thread and that's blocking us from
using shared worker contexts in the renderer.

[reveman, 2015-09-03 02:10:09]

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

[commit-bot: I haz the power, 2015-09-03 02:10:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/180001

[reveman, 2015-09-03 02:21:27]

I looked at removing ref counting from ContextProvider. Turned out to be a huge
undertaking and I'm not even sure that's what we want. After all, we want
sharing of ContextProviders between threads and have them be cleaned up when no
longer in use.

I went with a different approach:

::DetachFromThread makes it safe to both use and destroy the context on any
thread. This is done by invalidating any weak pointers used for callbacks at the
time ::DetachFromThread is called. I think this makes sense. Callbacks will run
on the thread that the context is bound to and once you detach it from the
thread there's no guarantee that the callbacks will run anymore. If needed in
the future (e.g. we want to use SignalQuery with worker contexts) then we can
add the ability to re-bind the context to a different thread (with the lock
held) to have the callbacks run on that thread.

PTAL

[commit-bot: I haz the power, 2015-09-03 03:39:15]

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

[commit-bot: I haz the power, 2015-09-03 03:39:15]

Dry run: This issue passed the CQ dry run.

[danakj, 2015-09-03 18:09:58]

I don't know enough to understand what kind of bad effects that might have or if
it's ok (like the context won't be removed from the share group?). So I'll let
piman or kbr comment :)

I like that this isn't so destructive to the shutdown code tho :)

https://codereview.chromium.org/1317743002/diff/180001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1317743002/diff/180001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:115: // that
callbacks such as ContextLost will be delivered after this has been
Can you add this to the ContextProvider::DetachFromThread comment too?

[piman, 2015-09-03 18:20:22]

On Wed, Sep 2, 2015 at 7:21 PM, <reveman@chromium.org> wrote:

> I looked at removing ref counting from ContextProvider. Turned out to be a
> huge
> undertaking and I'm not even sure that's what we want. After all, we want
> sharing of ContextProviders between threads and have them be cleaned up
> when no
> longer in use.
>
> I went with a different approach:
>
> ::DetachFromThread makes it safe to both use and destroy the context on any
> thread. This is done by invalidating any weak pointers used for callbacks
> at the
> time ::DetachFromThread is called. I think this makes sense. Callbacks
> will run
> on the thread that the context is bound to and once you detach it from the
> thread there's no guarantee that the callbacks will run anymore. If needed
> in
> the future (e.g. we want to use SignalQuery with worker contexts) then we
> can
> add the ability to re-bind the context to a different thread (with the lock
> held) to have the callbacks run on that thread.
>
> PTAL
>

There's more than the WebGraphicsContext3DCommandBufferImpl
callbacks, CommandBufferProxyImpl really needs to be destroyed on the
thread it was created... that's where messages get routed, and the critical
piece is that GpuChannelHost::MessageFilter uses a WeakPtr to
the CommandBufferProxyImpl, posting tasks to the original thread using that
WeakPtr. It needs to be invalidated on that thread.


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-09-03 18:24:39]

On Thu, Sep 3, 2015 at 11:20 AM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Wed, Sep 2, 2015 at 7:21 PM, <reveman@chromium.org> wrote:
>
>> I looked at removing ref counting from ContextProvider. Turned out to be
>> a huge
>> undertaking and I'm not even sure that's what we want. After all, we want
>> sharing of ContextProviders between threads and have them be cleaned up
>> when no
>> longer in use.
>>
>> I went with a different approach:
>>
>> ::DetachFromThread makes it safe to both use and destroy the context on
>> any
>> thread. This is done by invalidating any weak pointers used for callbacks
>> at the
>> time ::DetachFromThread is called. I think this makes sense. Callbacks
>> will run
>> on the thread that the context is bound to and once you detach it from the
>> thread there's no guarantee that the callbacks will run anymore. If
>> needed in
>> the future (e.g. we want to use SignalQuery with worker contexts) then we
>> can
>> add the ability to re-bind the context to a different thread (with the
>> lock
>> held) to have the callbacks run on that thread.
>>
>> PTAL
>>
>
> There's more than the WebGraphicsContext3DCommandBufferImpl
> callbacks, CommandBufferProxyImpl really needs to be destroyed on the
> thread it was created... that's where messages get routed, and the critical
> piece is that GpuChannelHost::MessageFilter uses a WeakPtr to
> the CommandBufferProxyImpl, posting tasks to the original thread using that
> WeakPtr. It needs to be invalidated on that thread.
>
>
Hm. Remind me.. do we have to be able to detach this context from a thread?
Can we just bind it only on the worker thread?


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-09-03 18:38:00]

On Thu, Sep 3, 2015 at 11:23 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Sep 3, 2015 at 11:20 AM, Antoine Labour <piman@chromium.org>
> wrote:
>
>>
>>
>> On Wed, Sep 2, 2015 at 7:21 PM, <reveman@chromium.org> wrote:
>>
>>> I looked at removing ref counting from ContextProvider. Turned out to be
>>> a huge
>>> undertaking and I'm not even sure that's what we want. After all, we want
>>> sharing of ContextProviders between threads and have them be cleaned up
>>> when no
>>> longer in use.
>>>
>>> I went with a different approach:
>>>
>>> ::DetachFromThread makes it safe to both use and destroy the context on
>>> any
>>> thread. This is done by invalidating any weak pointers used for
>>> callbacks at the
>>> time ::DetachFromThread is called. I think this makes sense. Callbacks
>>> will run
>>> on the thread that the context is bound to and once you detach it from
>>> the
>>> thread there's no guarantee that the callbacks will run anymore. If
>>> needed in
>>> the future (e.g. we want to use SignalQuery with worker contexts) then
>>> we can
>>> add the ability to re-bind the context to a different thread (with the
>>> lock
>>> held) to have the callbacks run on that thread.
>>>
>>> PTAL
>>>
>>
>> There's more than the WebGraphicsContext3DCommandBufferImpl
>> callbacks, CommandBufferProxyImpl really needs to be destroyed on the
>> thread it was created... that's where messages get routed, and the critical
>> piece is that GpuChannelHost::MessageFilter uses a WeakPtr to
>> the CommandBufferProxyImpl, posting tasks to the original thread using that
>> WeakPtr. It needs to be invalidated on that thread.
>>
>>
> Hm. Remind me.. do we have to be able to detach this context from a
> thread? Can we just bind it only on the worker thread?
>

1- the same context is used on multiple worker threads and the compositor
thread
2- a context needs to be bound to a TaskRunner, because we post tasks to
it. Workers aren't.

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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2015-09-03 21:34:33]

On 2015/09/03 at 18:38:00, piman wrote:
> On Thu, Sep 3, 2015 at 11:23 AM, Dana Jansens <danakj@chromium.org> wrote:
> 
> > On Thu, Sep 3, 2015 at 11:20 AM, Antoine Labour <piman@chromium.org>
> > wrote:
> >
> >>
> >>
> >> On Wed, Sep 2, 2015 at 7:21 PM, <reveman@chromium.org> wrote:
> >>
> >>> I looked at removing ref counting from ContextProvider. Turned out to be
> >>> a huge
> >>> undertaking and I'm not even sure that's what we want. After all, we want
> >>> sharing of ContextProviders between threads and have them be cleaned up
> >>> when no
> >>> longer in use.
> >>>
> >>> I went with a different approach:
> >>>
> >>> ::DetachFromThread makes it safe to both use and destroy the context on
> >>> any
> >>> thread. This is done by invalidating any weak pointers used for
> >>> callbacks at the
> >>> time ::DetachFromThread is called. I think this makes sense. Callbacks
> >>> will run
> >>> on the thread that the context is bound to and once you detach it from
> >>> the
> >>> thread there's no guarantee that the callbacks will run anymore. If
> >>> needed in
> >>> the future (e.g. we want to use SignalQuery with worker contexts) then
> >>> we can
> >>> add the ability to re-bind the context to a different thread (with the
> >>> lock
> >>> held) to have the callbacks run on that thread.
> >>>
> >>> PTAL
> >>>
> >>
> >> There's more than the WebGraphicsContext3DCommandBufferImpl
> >> callbacks, CommandBufferProxyImpl really needs to be destroyed on the
> >> thread it was created... that's where messages get routed, and the critical
> >> piece is that GpuChannelHost::MessageFilter uses a WeakPtr to
> >> the CommandBufferProxyImpl, posting tasks to the original thread using that
> >> WeakPtr. It needs to be invalidated on that thread.
> >>
> >>
> > Hm. Remind me.. do we have to be able to detach this context from a
> > thread? Can we just bind it only on the worker thread?
> >
> 
> 1- the same context is used on multiple worker threads and the compositor
> thread

and hopefully also the media thread in the near future. 

> 2- a context needs to be bound to a TaskRunner, because we post tasks to
> it. Workers aren't.

correct.

I'm having a hard time deciding what is the best approach for supporting the
general idea of a context that is shared between threads. Here are two
alternatives. If you can think of another, let me know.

1. Shared detached context provider. (this is the approach taken by the current
patch, although the patch is broken as it doesn't deal with ipc messages
correctly)
 - ContextProvider::BindToCurrentThread() - creates command buffer and binds it
to the current thread, callbacks and IPC messages will now go to the current
thread and only the current thread should keep references to the ContextProvider
as it must be destroyed on this thread. 
 - ContextProvider::DetachFromThread() - detaches the context from the current
thread and allows it to be used and destroyed on any thread. Callbacks and IPC
messages are dropped as the context is not attached to a thread but as a result
can be used and referenced on any thread.

2. Shared context provider that is always attached to one thread but access can
fail.
 - ContextProvider::BindToCurrentThread() - same as before.
 - ContextProvider::SetupLock() - Enables access from other threads using
"base::Lock* ContextProvider::AcquireLock()" and other threads can keep
references to the context.
 - ContextProvider::AcquireLock() - This can fail if the context has been
destroyed. All users of worker contexts need to be able to handle this failure.
e.g. the compositor will skip raster.
 - The thread for which the context is bound to is responsible for destroying
the context3d_ and putting the shared context in a state where AcquireLock will
fail.


I'm leaning towards option 2. Wdyt?

Btw, maybe support for SignalQuery (and sync point callbacks if needed) on
worker threads can be provided using a new "SignalQuery(uint32 query,
base::WaitableEvent* event)" function that is allowed to be used from a thread
other than the one the context is bound to.

[jbauman, 2015-09-03 22:17:57]

On 2015/09/03 21:34:33, reveman wrote:
> On 2015/09/03 at 18:38:00, piman wrote:
> > On Thu, Sep 3, 2015 at 11:23 AM, Dana Jansens <mailto:danakj@chromium.org>
wrote:
> > 
> > > On Thu, Sep 3, 2015 at 11:20 AM, Antoine Labour
<mailto:piman@chromium.org>
> > > wrote:
> > >
> > >>
> > >>
> > >> On Wed, Sep 2, 2015 at 7:21 PM, <mailto:reveman@chromium.org> wrote:
> > >>
> > >>> I looked at removing ref counting from ContextProvider. Turned out to be
> > >>> a huge
> > >>> undertaking and I'm not even sure that's what we want. After all, we
want
> > >>> sharing of ContextProviders between threads and have them be cleaned up
> > >>> when no
> > >>> longer in use.
> > >>>
> > >>> I went with a different approach:
> > >>>
> > >>> ::DetachFromThread makes it safe to both use and destroy the context on
> > >>> any
> > >>> thread. This is done by invalidating any weak pointers used for
> > >>> callbacks at the
> > >>> time ::DetachFromThread is called. I think this makes sense. Callbacks
> > >>> will run
> > >>> on the thread that the context is bound to and once you detach it from
> > >>> the
> > >>> thread there's no guarantee that the callbacks will run anymore. If
> > >>> needed in
> > >>> the future (e.g. we want to use SignalQuery with worker contexts) then
> > >>> we can
> > >>> add the ability to re-bind the context to a different thread (with the
> > >>> lock
> > >>> held) to have the callbacks run on that thread.
> > >>>
> > >>> PTAL
> > >>>
> > >>
> > >> There's more than the WebGraphicsContext3DCommandBufferImpl
> > >> callbacks, CommandBufferProxyImpl really needs to be destroyed on the
> > >> thread it was created... that's where messages get routed, and the
critical
> > >> piece is that GpuChannelHost::MessageFilter uses a WeakPtr to
> > >> the CommandBufferProxyImpl, posting tasks to the original thread using
that
> > >> WeakPtr. It needs to be invalidated on that thread.
> > >>
> > >>
> > > Hm. Remind me.. do we have to be able to detach this context from a
> > > thread? Can we just bind it only on the worker thread?
> > >
> > 
> > 1- the same context is used on multiple worker threads and the compositor
> > thread
> 
> and hopefully also the media thread in the near future. 
> 
> > 2- a context needs to be bound to a TaskRunner, because we post tasks to
> > it. Workers aren't.
> 
> correct.
> 
> I'm having a hard time deciding what is the best approach for supporting the
> general idea of a context that is shared between threads. Here are two
> alternatives. If you can think of another, let me know.
> 
> 1. Shared detached context provider. (this is the approach taken by the
current
> patch, although the patch is broken as it doesn't deal with ipc messages
> correctly)
>  - ContextProvider::BindToCurrentThread() - creates command buffer and binds
it
> to the current thread, callbacks and IPC messages will now go to the current
> thread and only the current thread should keep references to the
ContextProvider
> as it must be destroyed on this thread. 
>  - ContextProvider::DetachFromThread() - detaches the context from the current
> thread and allows it to be used and destroyed on any thread. Callbacks and IPC
> messages are dropped as the context is not attached to a thread but as a
result
> can be used and referenced on any thread.
> 
> 2. Shared context provider that is always attached to one thread but access
can
> fail.
>  - ContextProvider::BindToCurrentThread() - same as before.
>  - ContextProvider::SetupLock() - Enables access from other threads using
> "base::Lock* ContextProvider::AcquireLock()" and other threads can keep
> references to the context.
>  - ContextProvider::AcquireLock() - This can fail if the context has been
> destroyed. All users of worker contexts need to be able to handle this
failure.
> e.g. the compositor will skip raster.
>  - The thread for which the context is bound to is responsible for destroying
> the context3d_ and putting the shared context in a state where AcquireLock
will
> fail.
> 
> 
> I'm leaning towards option 2. Wdyt?

Yeah, #2 makes the most sense to me.
> 
> Btw, maybe support for SignalQuery (and sync point callbacks if needed) on
> worker threads can be provided using a new "SignalQuery(uint32 query,
> base::WaitableEvent* event)" function that is allowed to be used from a thread
> other than the one the context is bound to.

I think that should work.

[danakj, 2015-09-03 22:31:17]

Thanks for thinking so hard about this David. #2 sounds like a good
improvement if it works for the raster code.

On Thu, Sep 3, 2015 at 3:17 PM, <jbauman@chromium.org> wrote:

> On 2015/09/03 21:34:33, reveman wrote:
>
>> On 2015/09/03 at 18:38:00, piman wrote:
>> > On Thu, Sep 3, 2015 at 11:23 AM, Dana Jansens <mailto:
>> danakj@chromium.org>
>>
> wrote:
>
>> >
>> > > On Thu, Sep 3, 2015 at 11:20 AM, Antoine Labour
>>
> <mailto:piman@chromium.org>
>
>> > > wrote:
>>
>> > >
>> > >>
>> > >>
>> > >> On Wed, Sep 2, 2015 at 7:21 PM, <mailto:reveman@chromium.org> wrote:
>> > >>
>> > >>> I looked at removing ref counting from ContextProvider. Turned out
>> to be
>> > >>> a huge
>> > >>> undertaking and I'm not even sure that's what we want. After all, we
>>
> want
>
>> > >>> sharing of ContextProviders between threads and have them be
>> cleaned up
>> > >>> when no
>> > >>> longer in use.
>> > >>>
>> > >>> I went with a different approach:
>> > >>>
>> > >>> ::DetachFromThread makes it safe to both use and destroy the
>> context on
>> > >>> any
>> > >>> thread. This is done by invalidating any weak pointers used for
>> > >>> callbacks at the
>> > >>> time ::DetachFromThread is called. I think this makes sense.
>> Callbacks
>> > >>> will run
>> > >>> on the thread that the context is bound to and once you detach it
>> from
>> > >>> the
>> > >>> thread there's no guarantee that the callbacks will run anymore. If
>> > >>> needed in
>> > >>> the future (e.g. we want to use SignalQuery with worker contexts)
>> then
>> > >>> we can
>> > >>> add the ability to re-bind the context to a different thread (with
>> the
>> > >>> lock
>> > >>> held) to have the callbacks run on that thread.
>> > >>>
>> > >>> PTAL
>> > >>>
>> > >>
>> > >> There's more than the WebGraphicsContext3DCommandBufferImpl
>> > >> callbacks, CommandBufferProxyImpl really needs to be destroyed on the
>> > >> thread it was created... that's where messages get routed, and the
>>
> critical
>
>> > >> piece is that GpuChannelHost::MessageFilter uses a WeakPtr to
>> > >> the CommandBufferProxyImpl, posting tasks to the original thread
>> using
>>
> that
>
>> > >> WeakPtr. It needs to be invalidated on that thread.
>> > >>
>> > >>
>> > > Hm. Remind me.. do we have to be able to detach this context from a
>> > > thread? Can we just bind it only on the worker thread?
>> > >
>> >
>> > 1- the same context is used on multiple worker threads and the
>> compositor
>> > thread
>>
>
> and hopefully also the media thread in the near future.
>>
>
> > 2- a context needs to be bound to a TaskRunner, because we post tasks to
>> > it. Workers aren't.
>>
>
> correct.
>>
>
> I'm having a hard time deciding what is the best approach for supporting
>> the
>> general idea of a context that is shared between threads. Here are two
>> alternatives. If you can think of another, let me know.
>>
>
> 1. Shared detached context provider. (this is the approach taken by the
>>
> current
>
>> patch, although the patch is broken as it doesn't deal with ipc messages
>> correctly)
>>   - ContextProvider::BindToCurrentThread() - creates command buffer and
>> binds
>>
> it
>
>> to the current thread, callbacks and IPC messages will now go to the
>> current
>> thread and only the current thread should keep references to the
>>
> ContextProvider
>
>> as it must be destroyed on this thread.
>>   - ContextProvider::DetachFromThread() - detaches the context from the
>> current
>> thread and allows it to be used and destroyed on any thread. Callbacks
>> and IPC
>> messages are dropped as the context is not attached to a thread but as a
>>
> result
>
>> can be used and referenced on any thread.
>>
>
> 2. Shared context provider that is always attached to one thread but access
>>
> can
>
>> fail.
>>   - ContextProvider::BindToCurrentThread() - same as before.
>>   - ContextProvider::SetupLock() - Enables access from other threads using
>> "base::Lock* ContextProvider::AcquireLock()" and other threads can keep
>> references to the context.
>>   - ContextProvider::AcquireLock() - This can fail if the context has been
>> destroyed. All users of worker contexts need to be able to handle this
>>
> failure.
>
>> e.g. the compositor will skip raster.
>>   - The thread for which the context is bound to is responsible for
>> destroying
>> the context3d_ and putting the shared context in a state where AcquireLock
>>
> will
>
>> fail.
>>
>
>
> I'm leaning towards option 2. Wdyt?
>>
>
> Yeah, #2 makes the most sense to me.
>
> Btw, maybe support for SignalQuery (and sync point callbacks if needed) on
>> worker threads can be provided using a new "SignalQuery(uint32 query,
>> base::WaitableEvent* event)" function that is allowed to be used from a
>> thread
>> other than the one the context is bound to.
>>
>
> I think that should work.
>
>
>
> https://codereview.chromium.org/1317743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-09-03 23:41:56]

On Thu, Sep 3, 2015 at 3:25 PM, Dana Jansens <danakj@chromium.org> wrote:

> Thanks for thinking so hard about this David. #2 sounds like a good
> improvement if it works for the raster code.
>

#2 sgtm.

Antoine


>
> On Thu, Sep 3, 2015 at 3:17 PM, <jbauman@chromium.org> wrote:
>
>> On 2015/09/03 21:34:33, reveman wrote:
>>
>>> On 2015/09/03 at 18:38:00, piman wrote:
>>> > On Thu, Sep 3, 2015 at 11:23 AM, Dana Jansens <mailto:
>>> danakj@chromium.org>
>>>
>> wrote:
>>
>>> >
>>> > > On Thu, Sep 3, 2015 at 11:20 AM, Antoine Labour
>>>
>> <mailto:piman@chromium.org>
>>
>>> > > wrote:
>>>
>>> > >
>>> > >>
>>> > >>
>>> > >> On Wed, Sep 2, 2015 at 7:21 PM, <mailto:reveman@chromium.org>
>>> wrote:
>>> > >>
>>> > >>> I looked at removing ref counting from ContextProvider. Turned out
>>> to be
>>> > >>> a huge
>>> > >>> undertaking and I'm not even sure that's what we want. After all,
>>> we
>>>
>> want
>>
>>> > >>> sharing of ContextProviders between threads and have them be
>>> cleaned up
>>> > >>> when no
>>> > >>> longer in use.
>>> > >>>
>>> > >>> I went with a different approach:
>>> > >>>
>>> > >>> ::DetachFromThread makes it safe to both use and destroy the
>>> context on
>>> > >>> any
>>> > >>> thread. This is done by invalidating any weak pointers used for
>>> > >>> callbacks at the
>>> > >>> time ::DetachFromThread is called. I think this makes sense.
>>> Callbacks
>>> > >>> will run
>>> > >>> on the thread that the context is bound to and once you detach it
>>> from
>>> > >>> the
>>> > >>> thread there's no guarantee that the callbacks will run anymore. If
>>> > >>> needed in
>>> > >>> the future (e.g. we want to use SignalQuery with worker contexts)
>>> then
>>> > >>> we can
>>> > >>> add the ability to re-bind the context to a different thread (with
>>> the
>>> > >>> lock
>>> > >>> held) to have the callbacks run on that thread.
>>> > >>>
>>> > >>> PTAL
>>> > >>>
>>> > >>
>>> > >> There's more than the WebGraphicsContext3DCommandBufferImpl
>>> > >> callbacks, CommandBufferProxyImpl really needs to be destroyed on
>>> the
>>> > >> thread it was created... that's where messages get routed, and the
>>>
>> critical
>>
>>> > >> piece is that GpuChannelHost::MessageFilter uses a WeakPtr to
>>> > >> the CommandBufferProxyImpl, posting tasks to the original thread
>>> using
>>>
>> that
>>
>>> > >> WeakPtr. It needs to be invalidated on that thread.
>>> > >>
>>> > >>
>>> > > Hm. Remind me.. do we have to be able to detach this context from a
>>> > > thread? Can we just bind it only on the worker thread?
>>> > >
>>> >
>>> > 1- the same context is used on multiple worker threads and the
>>> compositor
>>> > thread
>>>
>>
>> and hopefully also the media thread in the near future.
>>>
>>
>> > 2- a context needs to be bound to a TaskRunner, because we post tasks to
>>> > it. Workers aren't.
>>>
>>
>> correct.
>>>
>>
>> I'm having a hard time deciding what is the best approach for supporting
>>> the
>>> general idea of a context that is shared between threads. Here are two
>>> alternatives. If you can think of another, let me know.
>>>
>>
>> 1. Shared detached context provider. (this is the approach taken by the
>>>
>> current
>>
>>> patch, although the patch is broken as it doesn't deal with ipc messages
>>> correctly)
>>>   - ContextProvider::BindToCurrentThread() - creates command buffer and
>>> binds
>>>
>> it
>>
>>> to the current thread, callbacks and IPC messages will now go to the
>>> current
>>> thread and only the current thread should keep references to the
>>>
>> ContextProvider
>>
>>> as it must be destroyed on this thread.
>>>   - ContextProvider::DetachFromThread() - detaches the context from the
>>> current
>>> thread and allows it to be used and destroyed on any thread. Callbacks
>>> and IPC
>>> messages are dropped as the context is not attached to a thread but as a
>>>
>> result
>>
>>> can be used and referenced on any thread.
>>>
>>
>> 2. Shared context provider that is always attached to one thread but
>>> access
>>>
>> can
>>
>>> fail.
>>>   - ContextProvider::BindToCurrentThread() - same as before.
>>>   - ContextProvider::SetupLock() - Enables access from other threads
>>> using
>>> "base::Lock* ContextProvider::AcquireLock()" and other threads can keep
>>> references to the context.
>>>   - ContextProvider::AcquireLock() - This can fail if the context has
>>> been
>>> destroyed. All users of worker contexts need to be able to handle this
>>>
>> failure.
>>
>>> e.g. the compositor will skip raster.
>>>   - The thread for which the context is bound to is responsible for
>>> destroying
>>> the context3d_ and putting the shared context in a state where
>>> AcquireLock
>>>
>> will
>>
>>> fail.
>>>
>>
>>
>> I'm leaning towards option 2. Wdyt?
>>>
>>
>> Yeah, #2 makes the most sense to me.
>>
>> Btw, maybe support for SignalQuery (and sync point callbacks if needed) on
>>> worker threads can be provided using a new "SignalQuery(uint32 query,
>>> base::WaitableEvent* event)" function that is allowed to be used from a
>>> thread
>>> other than the one the context is bound to.
>>>
>>
>> I think that should work.
>>
>>
>>
>> https://codereview.chromium.org/1317743002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2015-09-04 18:13:12]

reveman@chromium.org changed reviewers:
+ boliu@chromium.org

[reveman, 2015-09-04 18:13:13]

+boliu for android_webview/ and related code

Latest patch implements alternative #2. Doing that also meant adding shared
worker contexts and I've updated the description to reflect that. PTAL.

[danakj, 2015-09-04 18:28:07]

https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
cc/output/context_provider.h:114: // Return true if the context inside the
provider is no longer valid.
Can we maybe make this clear that this function is only to be used with a
ContextProvider that has been SetupLock()'d? And maybe we could DCHECK that?

[reveman, 2015-09-04 18:55:50]

https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
cc/output/context_provider.h:114: // Return true if the context inside the
provider is no longer valid.
On 2015/09/04 at 18:28:07, danakj wrote:
> Can we maybe make this clear that this function is only to be used with a
ContextProvider that has been SetupLock()'d? And maybe we could DCHECK that?

I prefer to have this function be consistent with the other functions of this
interface. That is, if the context provider is setup for usage on multiple
threads, then the lock must be held, if only used on the thread it's bound to
then there's no need to acquire the lock before calling this function.
Implementations typically have a
DCHECK(context_thread_checker_.CalledOnValidThread()) that will fail if used
incorrectly.

[reveman, 2015-09-04 19:59:50]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/280001

[danakj, 2015-09-04 20:02:15]

https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
cc/output/context_provider.h:114: // Return true if the context inside the
provider is no longer valid.
On 2015/09/04 18:55:50, reveman wrote:
> On 2015/09/04 at 18:28:07, danakj wrote:
> > Can we maybe make this clear that this function is only to be used with a
> ContextProvider that has been SetupLock()'d? And maybe we could DCHECK that?
> 
> I prefer to have this function be consistent with the other functions of this
> interface. That is, if the context provider is setup for usage on multiple
> threads, then the lock must be held, if only used on the thread it's bound to
> then there's no need to acquire the lock before calling this function.
> Implementations typically have a
> DCHECK(context_thread_checker_.CalledOnValidThread()) that will fail if used
> incorrectly.

My concern is that this adds a whole other state to the provider where
ContextGL() will be null. We don't want people having to worry about that state
(or causing that state) for contexts that aren't being used through the scoped
lock, do we? The idea of ContextProvider is that it's always usable even tho it
can do nothing if lost.

Having the ScopedLock return null is cool, but the way this is implemented in
https://codereview.chromium.org/1317743002/diff/280001/content/common/gpu/cli...
means that the ContextProvider will give back nulls too. Which is scarier?

[reveman, 2015-09-04 20:07:36]

On 2015/09/04 at 20:02:15, danakj wrote:
>
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
> File cc/output/context_provider.h (right):
> 
>
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
> cc/output/context_provider.h:114: // Return true if the context inside the
provider is no longer valid.
> On 2015/09/04 18:55:50, reveman wrote:
> > On 2015/09/04 at 18:28:07, danakj wrote:
> > > Can we maybe make this clear that this function is only to be used with a
> > ContextProvider that has been SetupLock()'d? And maybe we could DCHECK that?
> > 
> > I prefer to have this function be consistent with the other functions of
this
> > interface. That is, if the context provider is setup for usage on multiple
> > threads, then the lock must be held, if only used on the thread it's bound
to
> > then there's no need to acquire the lock before calling this function.
> > Implementations typically have a
> > DCHECK(context_thread_checker_.CalledOnValidThread()) that will fail if used
> > incorrectly.
> 
> My concern is that this adds a whole other state to the provider where
ContextGL() will be null. We don't want people having to worry about that state
(or causing that state) for contexts that aren't being used through the scoped
lock, do we? The idea of ContextProvider is that it's always usable even tho it
can do nothing if lost.
> 
> Having the ScopedLock return null is cool, but the way this is implemented in
https://codereview.chromium.org/1317743002/diff/280001/content/common/gpu/cli...
means that the ContextProvider will give back nulls too. Which is scarier?

How about we add a DCHECK (and comment) to ::Destroy() to make sure that's not
called unless ::SetupLock() has been called?

[danakj, 2015-09-04 20:17:21]

On 2015/09/04 20:07:36, reveman wrote:
> On 2015/09/04 at 20:02:15, danakj wrote:
> >
>
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
> > File cc/output/context_provider.h (right):
> > 
> >
>
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
> > cc/output/context_provider.h:114: // Return true if the context inside the
> provider is no longer valid.
> > On 2015/09/04 18:55:50, reveman wrote:
> > > On 2015/09/04 at 18:28:07, danakj wrote:
> > > > Can we maybe make this clear that this function is only to be used with
a
> > > ContextProvider that has been SetupLock()'d? And maybe we could DCHECK
that?
> > > 
> > > I prefer to have this function be consistent with the other functions of
> this
> > > interface. That is, if the context provider is setup for usage on multiple
> > > threads, then the lock must be held, if only used on the thread it's bound
> to
> > > then there's no need to acquire the lock before calling this function.
> > > Implementations typically have a
> > > DCHECK(context_thread_checker_.CalledOnValidThread()) that will fail if
used
> > > incorrectly.
> > 
> > My concern is that this adds a whole other state to the provider where
> ContextGL() will be null. We don't want people having to worry about that
state
> (or causing that state) for contexts that aren't being used through the scoped
> lock, do we? The idea of ContextProvider is that it's always usable even tho
it
> can do nothing if lost.
> > 
> > Having the ScopedLock return null is cool, but the way this is implemented
in
>
https://codereview.chromium.org/1317743002/diff/280001/content/common/gpu/cli...
> means that the ContextProvider will give back nulls too. Which is scarier?
> 
> How about we add a DCHECK (and comment) to ::Destroy() to make sure that's not
> called unless ::SetupLock() has been called?

Ya, that sounds good.

Another question, do we need both DestroyedOnMainThread and HasBeenDestroyed?

And this patch kinda overloads the destroyed_ variable. One meaning is the
context is lost. One is the context is null. Could
DestroyWithMainThreadLockAcquired just do what OnContextLost does? Or does it
have to actually kill the context3d_?

Is the rule that on the non-bound thread, you can't hold a ref on the
ContextProvider outside of holding the lock on it?

[commit-bot: I haz the power, 2015-09-04 20:28:41]

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

[commit-bot: I haz the power, 2015-09-04 20:28:42]

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

[reveman, 2015-09-04 20:45:14]

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

[commit-bot: I haz the power, 2015-09-04 20:45:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1317743002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1317743002/300001

[reveman, 2015-09-04 21:00:02]

On 2015/09/04 at 20:17:21, danakj wrote:
> On 2015/09/04 20:07:36, reveman wrote:
> > On 2015/09/04 at 20:02:15, danakj wrote:
> > >
> >
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
> > > File cc/output/context_provider.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
> > > cc/output/context_provider.h:114: // Return true if the context inside the
> > provider is no longer valid.
> > > On 2015/09/04 18:55:50, reveman wrote:
> > > > On 2015/09/04 at 18:28:07, danakj wrote:
> > > > > Can we maybe make this clear that this function is only to be used
with a
> > > > ContextProvider that has been SetupLock()'d? And maybe we could DCHECK
that?
> > > > 
> > > > I prefer to have this function be consistent with the other functions of
> > this
> > > > interface. That is, if the context provider is setup for usage on
multiple
> > > > threads, then the lock must be held, if only used on the thread it's
bound
> > to
> > > > then there's no need to acquire the lock before calling this function.
> > > > Implementations typically have a
> > > > DCHECK(context_thread_checker_.CalledOnValidThread()) that will fail if
used
> > > > incorrectly.
> > > 
> > > My concern is that this adds a whole other state to the provider where
> > ContextGL() will be null. We don't want people having to worry about that
state
> > (or causing that state) for contexts that aren't being used through the
scoped
> > lock, do we? The idea of ContextProvider is that it's always usable even tho
it
> > can do nothing if lost.
> > > 
> > > Having the ScopedLock return null is cool, but the way this is implemented
in
> >
https://codereview.chromium.org/1317743002/diff/280001/content/common/gpu/cli...
> > means that the ContextProvider will give back nulls too. Which is scarier?
> > 
> > How about we add a DCHECK (and comment) to ::Destroy() to make sure that's
not
> > called unless ::SetupLock() has been called?
> 
> Ya, that sounds good.

Done.

> 
> Another question, do we need both DestroyedOnMainThread and HasBeenDestroyed?

I think so DestroyedOnMainThread can always be called on the main thread, while
HasBeenDestroyed follows the standard rule for cc::ContextProvider functions:
called on the thread the provider is bound to or called while holding the lock
in the multi-threaded use case..

> 
> And this patch kinda overloads the destroyed_ variable. One meaning is the
context is lost. One is the context is null. Could
DestroyWithMainThreadLockAcquired just do what OnContextLost does? Or does it
have to actually kill the context3d_?

I'm having DestroyWithMainThreadLockAcquired do what it's currently doing to
keep the dtor logic the same as before. I think tear down order of provider
members is important.

The idea is that destroyed_ means that the context is at least lost. In
multi-threaded mode, context3d_ might or might not exist but it doesn't matter
from the clients pov, the context should not be used either way. 

> 
> Is the rule that on the non-bound thread, you can't hold a ref on the
ContextProvider outside of holding the lock on it?

No, in multi-threaded mode (SetupLock has been called) it's perfectly fine to
hold a ref to the ContextProvider on a different thread. It just can't be used
unless HasBeenDestroyed returns false. In single threaded mode, only the bound
thread is allowed to hold a ref (just like before we introduced the worker
context concept).

[commit-bot: I haz the power, 2015-09-04 21:16:48]

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

[commit-bot: I haz the power, 2015-09-04 21:16:49]

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

[danakj, 2015-09-04 21:23:15]

On Fri, Sep 4, 2015 at 2:00 PM, <reveman@chromium.org> wrote:

> On 2015/09/04 at 20:17:21, danakj wrote:
>
>> On 2015/09/04 20:07:36, reveman wrote:
>> > On 2015/09/04 at 20:02:15, danakj wrote:
>> > >
>> >
>>
>
>
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
>
>> > > File cc/output/context_provider.h (right):
>> > >
>> > >
>> >
>>
>
>
https://codereview.chromium.org/1317743002/diff/220001/cc/output/context_prov...
>
>> > > cc/output/context_provider.h:114: // Return true if the context
>> inside the
>> > provider is no longer valid.
>> > > On 2015/09/04 18:55:50, reveman wrote:
>> > > > On 2015/09/04 at 18:28:07, danakj wrote:
>> > > > > Can we maybe make this clear that this function is only to be used
>>
> with a
>
>> > > > ContextProvider that has been SetupLock()'d? And maybe we could
>> DCHECK
>>
> that?
>
>> > > >
>> > > > I prefer to have this function be consistent with the other
>> functions of
>> > this
>> > > > interface. That is, if the context provider is setup for usage on
>>
> multiple
>
>> > > > threads, then the lock must be held, if only used on the thread it's
>>
> bound
>
>> > to
>> > > > then there's no need to acquire the lock before calling this
>> function.
>> > > > Implementations typically have a
>> > > > DCHECK(context_thread_checker_.CalledOnValidThread()) that will
>> fail if
>>
> used
>
>> > > > incorrectly.
>> > >
>> > > My concern is that this adds a whole other state to the provider where
>> > ContextGL() will be null. We don't want people having to worry about
>> that
>>
> state
>
>> > (or causing that state) for contexts that aren't being used through the
>>
> scoped
>
>> > lock, do we? The idea of ContextProvider is that it's always usable
>> even tho
>>
> it
>
>> > can do nothing if lost.
>> > >
>> > > Having the ScopedLock return null is cool, but the way this is
>> implemented
>>
> in
>
>> >
>>
>
>
https://codereview.chromium.org/1317743002/diff/280001/content/common/gpu/cli...
>
>> > means that the ContextProvider will give back nulls too. Which is
>> scarier?
>> >
>> > How about we add a DCHECK (and comment) to ::Destroy() to make sure
>> that's
>>
> not
>
>> > called unless ::SetupLock() has been called?
>>
>
> Ya, that sounds good.
>>
>
> Done.
>
>
> Another question, do we need both DestroyedOnMainThread and
>> HasBeenDestroyed?
>>
>
> I think so DestroyedOnMainThread can always be called on the main thread,
> while
> HasBeenDestroyed follows the standard rule for cc::ContextProvider
> functions:
> called on the thread the provider is bound to or called while holding the
> lock
> in the multi-threaded use case..


Can you mention this in the comments in context_provider.h so it's clear
when people see these 2 methods what is going on?


>
>
> And this patch kinda overloads the destroyed_ variable. One meaning is the
>>
> context is lost. One is the context is null. Could
> DestroyWithMainThreadLockAcquired just do what OnContextLost does? Or does
> it
> have to actually kill the context3d_?
>
> I'm having DestroyWithMainThreadLockAcquired do what it's currently doing
> to
> keep the dtor logic the same as before. I think tear down order of provider
> members is important.
>
> The idea is that destroyed_ means that the context is at least lost. In
> multi-threaded mode, context3d_ might or might not exist but it doesn't
> matter
> from the clients pov, the context should not be used either way.


>
>
> Is the rule that on the non-bound thread, you can't hold a ref on the
>>
> ContextProvider outside of holding the lock on it?
>
> No, in multi-threaded mode (SetupLock has been called) it's perfectly fine
> to
> hold a ref to the ContextProvider on a different thread. It just can't be
> used
> unless HasBeenDestroyed returns false. In single threaded mode, only the
> bound
> thread is allowed to hold a ref (just like before we introduced the worker
> context concept).
>

Thanks, can you put all of this into comments in context_provider.h
(including the bit about context is at-least lost or maybe null etc)?


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-09-04 22:19:13]

I'm not sure I fully understand the exact locking semantics, especially around
destruction.

I'm somewhat concerned that ripping the shared context under the worker tasks's
feet could cause leaks.

I wonder if we could instead, on a lost context or compositor destruction,
synchronize with the worker tasks and make sure they don't hold a reference to
the context, and only then release the compositor thread reference. Wouldn't
that solve the threading issues?

https://codereview.chromium.org/1317743002/diff/240001/cc/output/context_prov...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1317743002/diff/240001/cc/output/context_prov...
cc/output/context_provider.h:115: virtual bool HasBeenDestroyed() = 0;
Can this be combined with DestroyedOnMainThread?
It seems that there's nothing different, except which thread it runs on and
whether or not the context lock is acquired.

https://codereview.chromium.org/1317743002/diff/240001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1317743002/diff/240001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:116: gl->DeleteQueriesEXT(1,
&query_id);
Does that mean we may leak resources?

https://codereview.chromium.org/1317743002/diff/240001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:516: gl->OrderingBarrierCHROMIUM();
Missing indents?

https://codereview.chromium.org/1317743002/diff/240001/cc/test/test_context_p...
File cc/test/test_context_provider.cc (right):

https://codereview.chromium.org/1317743002/diff/240001/cc/test/test_context_p...
cc/test/test_context_provider.cc:214: return destroyed_;
I think you need to take destroyed_lock_ here.

https://codereview.chromium.org/1317743002/diff/240001/content/browser/androi...
File content/browser/android/in_process/context_provider_in_process.cc (right):

https://codereview.chromium.org/1317743002/diff/240001/content/browser/androi...
content/browser/android/in_process/context_provider_in_process.cc:211: return
destroyed_;
I think you need to take destroyed_lock_.

https://codereview.chromium.org/1317743002/diff/240001/content/common/gpu/cli...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1317743002/diff/240001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:247: bool
ContextProviderCommandBuffer::HasBeenDestroyed() {
I think you need to acquire main_thread_lock_ here.

Also, can you assert that if there is a context lock, it must be acquired?

https://codereview.chromium.org/1317743002/diff/240001/content/common/gpu/cli...
content/common/gpu/client/context_provider_command_buffer.cc:255: if
(context3d_->GetCommandBufferProxy()) {
Can we assert that if there is a context lock, it must be acquired?

[reveman, 2015-09-08 23:21:19]

On 2015/09/04 at 22:19:13, piman wrote:
> I'm not sure I fully understand the exact locking semantics, especially around
destruction.
> 
> I'm somewhat concerned that ripping the shared context under the worker
tasks's feet could cause leaks.
> 
> I wonder if we could instead, on a lost context or compositor destruction,
synchronize with the worker tasks and make sure they don't hold a reference to
the context, and only then release the compositor thread reference. Wouldn't
that solve the threading issues?

Synchronizing with all users of the worker context sounds complicated and a
rather large change that might make it hard to use this worker context in 
media/ where we currently miss support of lost context handling.

While I think the current patch works correctly, after thinking more about this
I'm not very happy with this mechanism of having users of the worker context
check if it's still valid after acquiring the lock. I've created an alternative
patch that is closer to an earlier version of this cl. It instead tries to
destroy the context like we do in lost context situations and leaves it safe to
still use and be deleted it on other threads. It seems to be working Ok but I
might be missing something. I uploaded this new version as a new CL:
https://codereview.chromium.org/1322853005