gpu: Thread-safe command buffer state lookup.

gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813, 638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/1285660590d371fedced6253c43be569c2d054ee
Cr-Commit-Position: refs/heads/master@{#437651}

[sunnyps, 2016-12-02 02:42:49]

Description was changed from

==========
gpu: Thread-safe command buffer state lookup.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813
==========

to

==========
gpu: Thread-safe command buffer state lookup.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sunnyps, 2016-12-02 02:52:03]

This is not final but PTAL

jbauman@ pointed out that GetLastToken gets called a lot and must also be cached
in CommandBufferHelper.

For a working prototype that uses this:
https://codereview.chromium.org/2440093003/

Run with --enable-gpu-scheduler --enable-gpu-async-worker-context (works fine
without any/both flags as well)

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
gpu/command_buffer/client/cmd_buffer_helper.h:324: int32_t last_get_;
bikeshed: cached_get_offset_?

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/g...
File gpu/command_buffer/client/gpu_control.h (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/g...
gpu/command_buffer/client/gpu_control.h:96: virtual bool
IsFenceSyncReleased(uint64_t release) = 0;
Need a better name for this...

This is the only truly "fast" thread-safe method in CommandBufferProxyImpl.
Maybe the name should reflect that?

[sunnyps, 2016-12-02 03:20:50]

jbauman@ suggested that we use a separate shared state for release count as it
is independent of the other stuff in CommandBuffer::State.

[piman, 2016-12-02 19:21:48]

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
gpu/command_buffer/client/cmd_buffer_helper.cc:275:
command_buffer_->WaitForTokenInRange(token, token_);
Let's cache the new get (and token if we decide to?) here.

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
gpu/command_buffer/client/cmd_buffer_helper.h:324: int32_t last_get_;
On 2016/12/02 02:52:03, sunnyps wrote:
> bikeshed: cached_get_offset_?

I like cached_get_offset_

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/g...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/g...
gpu/command_buffer/client/gles2_implementation.cc:408: bool
GLES2Implementation::IsFenceSyncReleased(uint64_t release_count) {
nit: please add a comment that this can be called on any thread without locking,
so we can only access (read) constant fields (such as gpu_control_ - we may even
want to indicate that on the field).

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:248: int32_t
CommandBufferProxyImpl::GetLastToken() {
nit: maybe return GetLastState().token ?

Maybe we don't really need this call anymore, though. the cost of TryUpdateState
and the lock are orders of magnitude larger than the cost of copying the entire
gpu::CommandBuffer::State

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:352: OnGpuStateError();
I don't think OnGpuStateError is right, because that will post tasks and call
OnGpuControlLostContext many times. I think you only need to call
OnGpuControlLostContextMaybeReentrant here. the task to call
OnGpuControlLostContext will have been posted by the thread that set the error.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:383: OnGpuStateError();
Ditto

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:729: gpu::error::Error
CommandBufferProxyImpl::GetLastError() {
nit: same as GetLastToken, I don't think we need this any more.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:753: OnGpuStateError();
Ditto, I think you only want to call OnGpuControlLostContextMaybeReentrant

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:781: gpu::CommandBuffer::State
CommandBufferProxyImpl::TryUpdateState() {
I don't think it's necessary for this (and other TryUpdateState version) to
return State. I don't think any caller uses the return value. Also, because
they're all called with state_lock_ held, callers can (and do) use last_state_
without loss of consistency.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:956:
gpu_control_client_->OnGpuControlLostContext();
There are some paths where state_lock_ is held (e.g. coming from
OnChannelError->OnGpuAsyncMessageError->DisconnectChannel).
We don't want to keep that lock held while calling this callback, otherwise it
can lead to deadlocks if the client tries to e.g. call more GL functions from
there, or get the last state, etc.

Also, GpuChannelMsg_DestroyCommandBuffer is sync and can be arbitrary slow, so
it could hurt responsiveness of IsFenceSyncReleased (though admittedly this
would only happen on a context loss, maybe it's not a super big deal).

I think we should release state_lock_ before calling DisconnectChannel in
OnGpuAsyncMessageError.  This would also be consistent with the fact that it is
*not* held in the LockAndDisconnectChannel path.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/service/gpu_command...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/service/gpu_command...
gpu/ipc/service/gpu_command_buffer_stub.cc:951:
command_buffer_->SetReleaseCount(release);
nit: do this before ReleaseFenceSync, to ensure consistency -
sync_point_client_->ReleaseFenceSync will call callbacks and stuff (e.g.
SignalSyncToken's), so this would make sure that by the time they run
IsFenceSyncReleased() would return true.

[sunnyps, 2016-12-07 03:30:46]

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

[commit-bot: I haz the power, 2016-12-07 03:31:13]

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

[sunnyps, 2016-12-07 03:31:22]

PTAL

Will write up CL description tomorrow.

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
gpu/command_buffer/client/cmd_buffer_helper.cc:275:
command_buffer_->WaitForTokenInRange(token, token_);
On 2016/12/02 19:21:48, piman wrote:
> Let's cache the new get (and token if we decide to?) here.

Done.

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/c...
gpu/command_buffer/client/cmd_buffer_helper.h:324: int32_t last_get_;
On 2016/12/02 19:21:48, piman wrote:
> On 2016/12/02 02:52:03, sunnyps wrote:
> > bikeshed: cached_get_offset_?
> 
> I like cached_get_offset_

Done.

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/g...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/command_buffer/client/g...
gpu/command_buffer/client/gles2_implementation.cc:408: bool
GLES2Implementation::IsFenceSyncReleased(uint64_t release_count) {
On 2016/12/02 19:21:48, piman wrote:
> nit: please add a comment that this can be called on any thread without
locking,
> so we can only access (read) constant fields (such as gpu_control_ - we may
even
> want to indicate that on the field).

Done.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:248: int32_t
CommandBufferProxyImpl::GetLastToken() {
On 2016/12/02 19:21:48, piman wrote:
> nit: maybe return GetLastState().token ?
> 
> Maybe we don't really need this call anymore, though. the cost of
TryUpdateState
> and the lock are orders of magnitude larger than the cost of copying the
entire
> gpu::CommandBuffer::State

Got rid of both GetLastToken and GetLastError. This requires GetLastState to
call TryUpdateState but that is OK because we don't call it as often as before.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:352: OnGpuStateError();
On 2016/12/02 19:21:48, piman wrote:
> I don't think OnGpuStateError is right, because that will post tasks and call
> OnGpuControlLostContext many times. I think you only need to call
> OnGpuControlLostContextMaybeReentrant here. the task to call
> OnGpuControlLostContext will have been posted by the thread that set the
error.

DisconnectChannel (the posted task) early outs if it's already called once.
Nonetheless, I've changed it because every call to WaitForGetOffset/Token would
post a new task otherwise.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:383: OnGpuStateError();
On 2016/12/02 19:21:48, piman wrote:
> Ditto

Done.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:753: OnGpuStateError();
On 2016/12/02 19:21:48, piman wrote:
> Ditto, I think you only want to call OnGpuControlLostContextMaybeReentrant

Done.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:781: gpu::CommandBuffer::State
CommandBufferProxyImpl::TryUpdateState() {
On 2016/12/02 19:21:48, piman wrote:
> I don't think it's necessary for this (and other TryUpdateState version) to
> return State. I don't think any caller uses the return value. Also, because
> they're all called with state_lock_ held, callers can (and do) use last_state_
> without loss of consistency.

Done.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/client/command_buff...
gpu/ipc/client/command_buffer_proxy_impl.cc:956:
gpu_control_client_->OnGpuControlLostContext();
On 2016/12/02 19:21:48, piman wrote:
> There are some paths where state_lock_ is held (e.g. coming from
> OnChannelError->OnGpuAsyncMessageError->DisconnectChannel).
> We don't want to keep that lock held while calling this callback, otherwise it
> can lead to deadlocks if the client tries to e.g. call more GL functions from
> there, or get the last state, etc.
> 
> Also, GpuChannelMsg_DestroyCommandBuffer is sync and can be arbitrary slow, so
> it could hurt responsiveness of IsFenceSyncReleased (though admittedly this
> would only happen on a context loss, maybe it's not a super big deal).
> 
> I think we should release state_lock_ before calling DisconnectChannel in
> OnGpuAsyncMessageError.  This would also be consistent with the fact that it
is
> *not* held in the LockAndDisconnectChannel path.

Changed it so that the OnGpuAsyncMessageError releases state_lock_ when calling
DisconnectChannel.

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/service/gpu_command...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2550583002/diff/1/gpu/ipc/service/gpu_command...
gpu/ipc/service/gpu_command_buffer_stub.cc:951:
command_buffer_->SetReleaseCount(release);
On 2016/12/02 19:21:48, piman wrote:
> nit: do this before ReleaseFenceSync, to ensure consistency -
> sync_point_client_->ReleaseFenceSync will call callbacks and stuff (e.g.
> SignalSyncToken's), so this would make sure that by the time they run
> IsFenceSyncReleased() would return true.

Done.

[commit-bot: I haz the power, 2016-12-07 04:07:00]

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

[commit-bot: I haz the power, 2016-12-07 04:07:00]

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

[piman, 2016-12-07 20:30:13]

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_control.h (right):

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_control.h:105: // the lock provided by the client.
Are there implementations that are not thread safe?
I think that would be problematic, because GLES2Implementation assumes it is -
at least if we want to claim that ContextSupport::IsFenceSyncReleased is
thread-safe. Do we need to add some introspection
(GpuControl::HasThreadSafeSupport()) so that we can at least DCHECK?

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/ring_buffer.cc:151: if
(!helper_->HasTokenPassed(block.token) || block.state == IN_USE) break;
So, this would call GetLastState(), take the lock and do the TryUpdateState for
every single block, which seems very heavyweight for a call that is meant to be
reasonably fast.
Could we grab the latest token only once and then compare against it in the
loop?

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
File gpu/ipc/in_process_command_buffer.cc (right):

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:547: return last_state_;
nit: this whole function is fully equivalent to GetStateFast() - maybe combine?

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:619: GetLastState().error ==
gpu::error::kNoError) {
Should we avoid taking the lock twice (2 calls to GetLastState) per loop?
Something like:

State state = GetLastState();
while (!InRange(start, end, state.token) &&
       state.error == gpu::error::kNoError) {
  flush_event_.Wait();
  state = GetLastState();
}

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:630: GetLastState().error ==
gpu::error::kNoError) {
Ditto

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:1029: bool
InProcessCommandBuffer::IsFenceSyncReleased(uint64_t release) {
We will need this one to be thread-safe too for mus things. I think it should be
fairly straightforward (a lock around last_state_, similar to
CommandBufferProxyImpl). Maybe it's ok to leave as a TODO?

I guess that answers my question of whether or not there are non-thread-safe
implementations, but it means we need to be honest about it all the way to
ContextSupport - and maybe provide a way to introspect, or at least DCHECK that
capability.

[sunnyps, 2016-12-08 00:59:19]

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

[commit-bot: I haz the power, 2016-12-08 00:59:50]

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

[sunnyps, 2016-12-08 01:14:16]

PTAL

Made ContextProviderCommandBuffer dump memory trace (under lock) from GLES2Impl
instead of directly registering GLES2Impl as a memory dump provider.
Made InProcessCommandBuffer::GetLastState thread-safe.
Renamed state_lock_ to last_state_lock_.

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_control.h (right):

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_control.h:105: // the lock provided by the client.
On 2016/12/07 20:30:12, piman wrote:
> Are there implementations that are not thread safe?

I made the InProcessCommandBuffer implementation thread-safe. Ppapi and test
only implementations, GLManager and MockClientGpuControl don't implement this
but they don't implement SetLock and a bunch of other things.

> I think that would be problematic, because GLES2Implementation assumes it is -
> at least if we want to claim that ContextSupport::IsFenceSyncReleased is
> thread-safe. Do we need to add some introspection
> (GpuControl::HasThreadSafeSupport()) so that we can at least DCHECK?

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/2550583002/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/ring_buffer.cc:151: if
(!helper_->HasTokenPassed(block.token) || block.state == IN_USE) break;
On 2016/12/07 20:30:12, piman wrote:
> So, this would call GetLastState(), take the lock and do the TryUpdateState
for
> every single block, which seems very heavyweight for a call that is meant to
be
> reasonably fast.
> Could we grab the latest token only once and then compare against it in the
> loop?

I made HasTokenPassed faster by using the cached token first and then updating
the state if necessary. I don't like exposing the token directly because clients
might not perform the wrap-around check that HasTokenPassed does. Also, clients
have no way of doing the cached token early out.

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
File gpu/ipc/in_process_command_buffer.cc (right):

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:619: GetLastState().error ==
gpu::error::kNoError) {
On 2016/12/07 20:30:13, piman wrote:
> Should we avoid taking the lock twice (2 calls to GetLastState) per loop?
> Something like:
> 
> State state = GetLastState();
> while (!InRange(start, end, state.token) &&
>        state.error == gpu::error::kNoError) {
>   flush_event_.Wait();
>   state = GetLastState();
> }

Done.

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:630: GetLastState().error ==
gpu::error::kNoError) {
On 2016/12/07 20:30:13, piman wrote:
> Ditto

Done.

https://codereview.chromium.org/2550583002/diff/20001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:1029: bool
InProcessCommandBuffer::IsFenceSyncReleased(uint64_t release) {
On 2016/12/07 20:30:13, piman wrote:
> We will need this one to be thread-safe too for mus things. I think it should
be
> fairly straightforward (a lock around last_state_, similar to
> CommandBufferProxyImpl). Maybe it's ok to leave as a TODO?
> 
> I guess that answers my question of whether or not there are non-thread-safe
> implementations, but it means we need to be honest about it all the way to
> ContextSupport - and maybe provide a way to introspect, or at least DCHECK
that
> capability.

Made this thread-safe although there's no real benefit to it right now because
last_state_ is only updated after a flush. We can make last_state_ SharedState
if we need to.

[piman, 2016-12-08 01:21:47]

lgtm

https://codereview.chromium.org/2550583002/diff/60001/gpu/ipc/in_process_comm...
File gpu/ipc/in_process_command_buffer.cc (right):

https://codereview.chromium.org/2550583002/diff/60001/gpu/ipc/in_process_comm...
gpu/ipc/in_process_command_buffer.cc:650: last_put_offset_ = 0;
Nice catch!

[ericrk, 2016-12-08 01:59:38]

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

[ericrk, 2016-12-08 01:59:39]

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

https://codereview.chromium.org/2550583002/diff/60001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:457:
gles2_impl_->OnMemoryDump(args, pmd);
Seems like this will fix crbug.com/638862, can you close that out when this
lands? Thanks!

[sunnyps, 2016-12-09 02:25:40]

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

[commit-bot: I haz the power, 2016-12-09 02:26:22]

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

[sunnyps, 2016-12-09 02:32:27]

PTAL

Made the pixel readback test call PerformIdleWork so that the callbacks which
cause the query to complete are called.

Also moved the memory dump for cmd buffer helper to context provider and checked
that there are no other gl helpers registered with memory dump manager directly.

[jbauman, 2016-12-09 03:16:04]

lgtm with a few nits.

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/cmd_buffer_helper.cc:279: return;
We should try to update the state here one last time before flushing.

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:151: if
(!helper_->HasTokenPassed(block.token) || block.state == IN_USE) break;
This will most likely hit the lock at least once per call to RingBuffer::Alloc,
as if any ringbuffer entry is being used on the service side it'll free blocks
until it hits that and has to check its token. That would probably be
acceptable, and I suspect the perf bots will let us know if not.

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/command_buffer.h:27: generation(0) {
initialize release_count

[sunnyps, 2016-12-09 04:08:25]

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

[commit-bot: I haz the power, 2016-12-09 04:08:47]

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

[sunnyps, 2016-12-09 04:09:37]

Thanks!

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/cmd_buffer_helper.cc:279: return;
On 2016/12/09 03:16:03, jbauman wrote:
> We should try to update the state here one last time before flushing.

Done.

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/ring_buffer.cc (right):

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/ring_buffer.cc:151: if
(!helper_->HasTokenPassed(block.token) || block.state == IN_USE) break;
On 2016/12/09 03:16:03, jbauman wrote:
> This will most likely hit the lock at least once per call to
RingBuffer::Alloc,
> as if any ringbuffer entry is being used on the service side it'll free blocks
> until it hits that and has to check its token. That would probably be
> acceptable, and I suspect the perf bots will let us know if not.

I didn't expose token from the helper directly because this wasn't handling
token wrap around and I wanted to avoid a similar mistake in the future.
HasTokenPassed has an early out if the cached token >= given token.

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/2550583002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/command_buffer.h:27: generation(0) {
On 2016/12/09 03:16:03, jbauman wrote:
> initialize release_count

Done.

[commit-bot: I haz the power, 2016-12-09 05:31:06]

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

[commit-bot: I haz the power, 2016-12-09 05:31:08]

Dry run: This issue passed the CQ dry run.

[sunnyps, 2016-12-09 20:11:38]

Description was changed from

==========
gpu: Thread-safe command buffer state lookup.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813,638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sunnyps, 2016-12-09 20:12:05]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-12-09 20:12:06]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2550583002/#ps120001
(title: "jbauman's review")

[commit-bot: I haz the power, 2016-12-09 20:13:10]

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

[commit-bot: I haz the power, 2016-12-09 21:06:48]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481314325823860,
"parent_rev": "f47102d5c0e2e49347c0e4970d334c99bdb77f6d", "commit_rev":
"53b4bdda6f2cb5540442dda3a66995d481cfed1b"}

[commit-bot: I haz the power, 2016-12-09 21:07:18]

Description was changed from

==========
gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813,638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813,638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2550583002
==========

[commit-bot: I haz the power, 2016-12-09 21:07:19]

Committed patchset #7 (id:120001)

[sunnyps, 2016-12-10 01:06:29]

Description was changed from

==========
gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813,638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2550583002
==========

to

==========
gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813,638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2550583002
Cr-Commit-Position: refs/heads/master@{#437651}
==========

[sunnyps, 2016-12-10 01:07:24]

This landed as 1285660590d371fedced6253c43be569c2d054ee (r437651) but commit bot
hasn't updated this CL for some reason :/

[commit-bot: I haz the power, 2016-12-12 14:40:48]

Description was changed from

==========
gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813,638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2550583002
Cr-Commit-Position: refs/heads/master@{#437651}
==========

to

==========
gpu: Thread-safe command buffer state lookup.

This makes client side (in-process / proxy) command buffer state thread-
safe and adds release count to the client-server shared state. This
allows the compositor thread to monitor worker context sync token in the
service.

Client side command buffer state is synchronized using a lock. Extra
caching of the state is added to command buffer helper so that it
doesn't acquire the lock for every command.

Also fixes command buffer memory tracing so that it happens on the same
thread which the context provider is bound to.

R=piman@chromium.org,jbauman@chromium.org
BUG=514813,638862
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/1285660590d371fedced6253c43be569c2d054ee
Cr-Commit-Position: refs/heads/master@{#437651}
==========