Use shared memory to update the renderer's view of the command buffer state.

Use shared memory to update the renderer's view of the command buffer state.

The renderer can't receive UpdateState messages while it's executing javascript or NaCl, causing it to eventually flushsync once it fills up the command buffer or transfer buffer. To avoid this, share a piece of memory between the renderer and gpu process that the GPU can asynchronously update the state. A 4-slot asynchronous communication mechanism is used so that the renderer always receives a consistent copy of the state that was put in by the GPU process.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122034

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=122593

[greggman, 2012-02-11 04:17:21]

This is awesome!

http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
File content/common/gpu/client/command_buffer_proxy.cc (right):

http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
content/common/gpu/client/command_buffer_proxy.cc:114: int32 state_buffer =
CreateTransferBuffer(sizeof *shared_state_, -1);
Just asking but does this need to be a separate piece of shared memory? Maybe it
does because it's being setup before GLES2Implemenation but I thought I'd ask.

The GLES2Implementation allocations a chunk of shared memory and currently
reserves the first 64 bytes for "results". That is defined by
GLES2Implemetation:kStartingOffset, from 0 to starting offset is the results
buffer, after that is the command buffer.

Would it be possible to just increase that size for these values?

Allocating separate shared memory means another file descriptor in linux which
we ran out of once before. It also means all this extra setup and 2 more sync
IPCs to startup where as if you can just use the existing memory you don't need
this extra setup.

If that's possible, I believe you can just increase
GLES2Implementation:kStartingOffset.  You can either put the state at 64 bytes
in, or you can put it at 0 bytes in and make
GLES2Implementation::GetResultOffset return enough of an offset to skip the
state.

If not no biggy.

http://codereview.chromium.org/9380037/diff/13/gpu/command_buffer/common/comm...
File gpu/command_buffer/common/command_buffer_shared.h (right):

http://codereview.chromium.org/9380037/diff/13/gpu/command_buffer/common/comm...
gpu/command_buffer/common/command_buffer_shared.h:9: #include "base/atomicops.h"
I don't think you can include base from common  or client as both are used in
untrusted Pepper code I think.  You could if def them out for NaCl if there's a
fallback?

[jbauman, 2012-02-11 04:38:18]

On 2012/02/11 04:17:21, greggman wrote:
> This is awesome!
> 
>
http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
> File content/common/gpu/client/command_buffer_proxy.cc (right):
> 
>
http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
> content/common/gpu/client/command_buffer_proxy.cc:114: int32 state_buffer =
> CreateTransferBuffer(sizeof *shared_state_, -1);
> Just asking but does this need to be a separate piece of shared memory? Maybe
it
> does because it's being setup before GLES2Implemenation but I thought I'd ask.
> 
> The GLES2Implementation allocations a chunk of shared memory and currently
> reserves the first 64 bytes for "results". That is defined by
> GLES2Implemetation:kStartingOffset, from 0 to starting offset is the results
> buffer, after that is the command buffer.
> 
> Would it be possible to just increase that size for these values?
> 
> Allocating separate shared memory means another file descriptor in linux which
> we ran out of once before. It also means all this extra setup and 2 more sync
> IPCs to startup where as if you can just use the existing memory you don't
need
> this extra setup.
> 
> If that's possible, I believe you can just increase
> GLES2Implementation:kStartingOffset.  You can either put the state at 64 bytes
> in, or you can put it at 0 bytes in and make
> GLES2Implementation::GetResultOffset return enough of an offset to skip the
> state.
> 
> If not no biggy.
> 

Yeah, I originally tried this, but the way the code is architected with a
transfer buffer that can come and go and the CommandBuffer having no idea which
transfer buffer contains the shared state made it a pretty messy change. Doing
it this way is a lot more straightforward, and hopefully that extra file
descriptor isn't too expensive. Though, I may try it that way again to see if I
can find a way to make it elegant.
>
http://codereview.chromium.org/9380037/diff/13/gpu/command_buffer/common/comm...
> File gpu/command_buffer/common/command_buffer_shared.h (right):
> 
>
http://codereview.chromium.org/9380037/diff/13/gpu/command_buffer/common/comm...
> gpu/command_buffer/common/command_buffer_shared.h:9: #include
"base/atomicops.h"
> I don't think you can include base from common  or client as both are used in
> untrusted Pepper code I think.  You could if def them out for NaCl if there's
a
> fallback?

At the moment this code isn't included at all in pepper, as they don't have
their own CommandBufferProxy and instead do IPCs to the one in the renderer to
get the state. If I can find a working replacement for these atomic operations,
I'll include it and use it there.

[greggman, 2012-02-11 04:51:04]

Sounds good.

Al should review this more than me so get his blessing.

I have a CL that does tries async flushes transfers.

[apatrick_chromium, 2012-02-13 20:50:58]

Awesome. A few questions...

http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
File content/common/gpu/client/command_buffer_proxy.cc (right):

http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
content/common/gpu/client/command_buffer_proxy.cc:130: if (!Send(new
GpuCommandBufferMsg_SetSharedStateBuffer(route_id_,
Could this be sent over as another argument to GpuCommandBufferMsg_Initialize?

http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
File content/common/gpu/client/command_buffer_proxy.h (right):

http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
content/common/gpu/client/command_buffer_proxy.h:133: State last_state_;
Do we need to have two copied of the shared state? It seems redundant.

[jbauman, 2012-02-14 01:28:07]

On 2012/02/13 20:50:58, apatrick_chromium wrote:
> Awesome. A few questions...
> 
>
http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
> File content/common/gpu/client/command_buffer_proxy.cc (right):
> 
>
http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
> content/common/gpu/client/command_buffer_proxy.cc:130: if (!Send(new
> GpuCommandBufferMsg_SetSharedStateBuffer(route_id_,
> Could this be sent over as another argument to GpuCommandBufferMsg_Initialize?
> 
I wanted to reuse our transfer buffer mechanism for convenience, but doing it
that way is probably possible.
>
http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
> File content/common/gpu/client/command_buffer_proxy.h (right):
> 
>
http://codereview.chromium.org/9380037/diff/13/content/common/gpu/client/comm...
> content/common/gpu/client/command_buffer_proxy.h:133: State last_state_;
> Do we need to have two copied of the shared state? It seems redundant.

Getting the shared state takes a bit of time, so I'd prefer to avoid doing it in
really hot cases, like GetLastState from get_offset() in CommandBufferHelper.

[apatrick_chromium, 2012-02-14 01:31:56]

lgtm

[commit-bot: I haz the power, 2012-02-15 00:48:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/9380037/13

[commit-bot: I haz the power, 2012-02-15 01:27:21]

Try job failure for 9380037-13 (retry) on linux_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-15 01:31:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/9380037/13

[commit-bot: I haz the power, 2012-02-15 04:12:59]

Change committed as 122034