cc: Implement shared worker contexts.

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 can now be explicitly destroyed on the thread they are
bound to. Destroying a context is similar to losing it. It is not
required by a user of the context to check if it has been lost/destroyed.

BUG=523411
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[reveman, 2015-09-08 20: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-08 20:18:24]

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

[commit-bot: I haz the power, 2015-09-08 20:55:13]

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

[commit-bot: I haz the power, 2015-09-08 20:55:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[reveman, 2015-09-08 21:54:51]

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

[commit-bot: I haz the power, 2015-09-08 21:55:08]

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

[commit-bot: I haz the power, 2015-09-08 22:49:27]

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

[commit-bot: I haz the power, 2015-09-08 22:49:28]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

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

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

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

V2 of shared worker contexts. I think I now prefer this approach as it keeps the
code that use the GLES2Interface clean. piman, afaict deleting the command
buffer proxy impl on any thread works OK as long as we unregister all callbacks
in the thread it's bound to. What do you think about the current patch and this
approach?

[Ken Russell (switch to Gerrit), 2015-09-09 06:09:11]

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

[Ken Russell (switch to Gerrit), 2015-09-09 06:09:11]

Apologies for not keeping up with the reviews to date.

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:55:
channel_->DestroyCommandBuffer(this);
Doesn't DestroyCommandBuffer need to be called on the same thread that created
the CommandBufferProxyImpl so that the IPC route set up by the GpuChannelHost is
torn down correctly? If so, is that guaranteed?

[reveman, 2015-09-09 13:45:24]

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:55:
channel_->DestroyCommandBuffer(this);
On 2015/09/09 at 06:09:11, Ken Russell wrote:
> Doesn't DestroyCommandBuffer need to be called on the same thread that created
the CommandBufferProxyImpl so that the IPC route set up by the GpuChannelHost is
torn down correctly? If so, is that guaranteed?

Correct. ::Destroy has the same thread usage restrictions as the dtor. In the
case of shared worker contexts this is always the main thread.

An explicit Destroy function makes it possible to always call
DestroyCommandBuffer on the correct thread instead of relying on the last
reference to the ContextProvider to be dropped on the correct thread, which in
the case of shared worker contexts is hard to guarantee in a clean way.

[piman, 2015-09-09 19:14:23]

I think there are still problems.

I'm still not excited about the Destroy() primitive being exposed at the
ContextProvider level. The correct use cases are extremely restricted if you
want to avoid leaks and races - it either has to be the only context in its
share group, or it needs to be already lost (along with other contexts in teh
share group), or the caller has to be the last user - and this is not something
we can easily DCHECK for.

I still don't understand why it's hard to ensure tasks are done before resetting
the context on the compositor thread. We actually already do that AFAICT, in
LayerTreeHostImpl::InitializeRenderer and ~LayerTreeHostImpl: CleanUpTileManager
will wait for all tasks before we reset output_surface_ (which should drop its
reference to the ContextProvider).

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:51: void
CommandBufferProxyImpl::Destroy() {
The lock needs to be held here. Should this CheckLock()?

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:57: last_state_.error =
gpu::error::kLostContext;
If we lose the context, we have to lose all contexts in the same share group.
This is necessary per spec, and is necessary to avoid resource leaks and races.

leaks: resources are per share group. After a context is lost, it can't send
commands to release its resources. Other clients that use the same share group
most likely don't know about these resources and can't release them on its
behalf. The only way to release these resources is to destroy all contexts in
this share group - by reporting all of them lost.

races: if some contexts report lost but others don't, some clients will try to
recreate whereas others won't. That results in different clients using different
share groups, whereas they believe they do.


If the context is lost on the GPU side, the GPU process ensures it will cause
all contexts to be lost. If the channel is lost, we also ensure all contexts on
the same channel will report being lost.

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/1322853005/diff/20001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:255:
context3d_->GetCommandBufferProxy()->SetLock(nullptr);
That's unsafe if the lock wasn't taken (or this is the only thread). Another
thread might be checking whether lock_ is set on the context.

I'm not convinced that this is ok. If you reset the lock (even while it is held,
and even if on the context thread), won't IPC messages after that fail to take
the lock, causing data races on some of CommandBufferProxyImpl members if
read/modified on other threads, even though they take the lock?

[reveman, 2015-09-09 20:46:18]

On 2015/09/09 at 19:14:23, piman wrote:
> I think there are still problems.
> 
> I'm still not excited about the Destroy() primitive being exposed at the
ContextProvider level. The correct use cases are extremely restricted if you
want to avoid leaks and races - it either has to be the only context in its
share group, or it needs to be already lost (along with other contexts in teh
share group), or the caller has to be the last user - and this is not something
we can easily DCHECK for.

Ok, got it. Leaking resources in the share group is definitely a problem with
the current patch. Thanks for explaining.

> 
> I still don't understand why it's hard to ensure tasks are done before
resetting the context on the compositor thread. We actually already do that
AFAICT, in LayerTreeHostImpl::InitializeRenderer and ~LayerTreeHostImpl:
CleanUpTileManager will wait for all tasks before we reset output_surface_
(which should drop its reference to the ContextProvider).

Synchronizing tasks and usage is not a problem and already happening as you
pointed out. The problem is when to create a new shared worker context. When you
said synchronizing I thought you meant synchronizing all users (all compositors
+ media/ in the near future) of the worker context so we can destroy the old
context and create a new one and that's what seem complicated to me. However, I
don't think that's necessary. I think we just need to do whatever it takes to
make ContextProvider RefCounted instead of RefCountedThreadSafe. I'll look at
what's need for this. Guessing all OutputSurfaces will need to be destroyed on
the same thread they were created on, which probably means adding an
OutputSurface::DetachFromClient function to destroy the non-worker context on
the compositor thread.

I'll let you know when I have something new to review.

[piman, 2015-09-09 20:59:36]

On Wed, Sep 9, 2015 at 1:46 PM, <reveman@chromium.org> wrote:

> On 2015/09/09 at 19:14:23, piman wrote:
>
>> I think there are still problems.
>>
>
> I'm still not excited about the Destroy() primitive being exposed at the
>>
> ContextProvider level. The correct use cases are extremely restricted if
> you
> want to avoid leaks and races - it either has to be the only context in its
> share group, or it needs to be already lost (along with other contexts in
> teh
> share group), or the caller has to be the last user - and this is not
> something
> we can easily DCHECK for.
>
> Ok, got it. Leaking resources in the share group is definitely a problem
> with
> the current patch. Thanks for explaining.
>
>
> I still don't understand why it's hard to ensure tasks are done before
>>
> resetting the context on the compositor thread. We actually already do that
> AFAICT, in LayerTreeHostImpl::InitializeRenderer and ~LayerTreeHostImpl:
> CleanUpTileManager will wait for all tasks before we reset output_surface_
> (which should drop its reference to the ContextProvider).
>
> Synchronizing tasks and usage is not a problem and already happening as you
> pointed out. The problem is when to create a new shared worker context.
> When you
> said synchronizing I thought you meant synchronizing all users (all
> compositors
> + media/ in the near future) of the worker context so we can destroy the
> old
> context and create a new one and that's what seem complicated to me.
> However, I
> don't think that's necessary.


Right, contexts can co-exist, and giving the new context to new systems can
be "pipelined", provided we have the guarantee that contexts are deleted on
their correct thread.
As you point out, what is needed though is to detect the lost state in the
place where we need to decide whether to recreate one or not (i.e. the main
thread). For the shared context, I think it's ok to grab the context lock
while querying.


> I think we just need to do whatever it takes to
> make ContextProvider RefCounted instead of RefCountedThreadSafe. I'll look
> at
> what's need for this.


Ok, just beware that we probably use the RefCountedThreadSafe property to
post tasks before we make the BindToCurrentThread
(before BindToCurrentThread is called, it's safe to destroy the
ContextProvider on any thread - for better or worse).


> Guessing all OutputSurfaces will need to be destroyed on
> the same thread they were created on, which probably means adding an
> OutputSurface::DetachFromClient function to destroy the non-worker context
> on
> the compositor thread.
>
> I'll let you know when I have something new to review.
>
> https://codereview.chromium.org/1322853005/
>

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