Add OrderingBarrierCHROMIUM API.

Add OrderingBarrierCHROMIUM API.

Implements a GPU Channel level command buffer barrier, which ensures
ordering between channels, without immediately notifying the GPU service.

Multiple Ordering Barriers can be combined into single IPCs to the GPU
service, thus this API can be used in place of ShallowFlushCHROMIUM, to
reduce IPC count and GPU service overhead.

BUG=454500
Committed: https://crrev.com/b700b43d82aee159ade4db622dea6bdd920a7d7b
Cr-Commit-Position: refs/heads/master@{#315048}

[vmiura, 2015-02-04 04:03:45]

vmiura@chromium.org changed reviewers:
+ jbauman@chromium.org, piman@chromium.org

[vmiura, 2015-02-04 04:03:45]

PTAL.

[vmiura, 2015-02-04 05:29:07]

Patchset #1 (id:1) has been deleted

[jbauman, 2015-02-05 00:23:02]

On 2015/02/04 04:03:45, vmiura wrote:
> PTAL.

Overall lgtm, but the name needs some work. Maybe DeferredFlush or
CoalescingFlush?

[piman, 2015-02-05 01:14:15]

Re name: OrderingBarrier() ? Implying it ensures order, but doesn't really
"flush" anything (doesn't ensure eventual execution).

https://codereview.chromium.org/896723008/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/896723008/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:92: return InternalSend(msg);
The main thing is that some of the messages are synchronous, and keeping the
lock while sending those is unfortunate (e.g. if it's a
WaitForGetOffsetInRange/WaitForTokenInRange which can be arbitrary long).

I /think/ it's ok to release after the InternalFlush, but it's subtle. The
argument is that if there's 2 (or more) threads trying to send a message "at the
same time" (without mutual exclusion), then they don't have an ordering
dependency between them (because nothing currently would guarantee ordering).
The only potential dependencies are between any previous Flush and each of the
message. The first one of these messages gets to grab the lock, and ensures that
the Flush will happen before that message, but also before any other message
will go through.

https://codereview.chromium.org/896723008/diff/40001/gpu/command_buffer/commo...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/896723008/diff/40001/gpu/command_buffer/commo...
gpu/command_buffer/common/command_buffer.h:93: // As Flush, ensuress that on the
service side, commands up to put_offset
typo: ensuress

[vmiura, 2015-02-05 01:20:49]

https://codereview.chromium.org/896723008/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/896723008/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:92: return InternalSend(msg);
Makes sense.

I think doing InternalFlush on every Send may be overkill.  Certain IPCs must be
ordered relative to the pending Flush(), but I'm not sure which.

For one, would it be sufficient to run InternalFlush() when the Send is from the
same route_id as the pending Flush?

Then, are there only specific IPCs within the route that need strict ordering
with the Flush?

[piman, 2015-02-05 01:59:10]

On 2015/02/05 01:20:49, vmiura wrote:
>
https://codereview.chromium.org/896723008/diff/40001/content/common/gpu/clien...
> File content/common/gpu/client/gpu_channel_host.cc (right):
> 
>
https://codereview.chromium.org/896723008/diff/40001/content/common/gpu/clien...
> content/common/gpu/client/gpu_channel_host.cc:92: return InternalSend(msg);
> Makes sense.
> 
> I think doing InternalFlush on every Send may be overkill.  Certain IPCs must
be
> ordered relative to the pending Flush(), but I'm not sure which.
> 
> For one, would it be sufficient to run InternalFlush() when the Send is from
the
> same route_id as the pending Flush?
> 
> Then, are there only specific IPCs within the route that need strict ordering
> with the Flush?

Since InternalFlush is cheap in the common case, maybe it's ok?
Weakening IPC ordering is kind of scary, we need to be very careful. Most IPCs
if not all on the same route definitely need ordering wrt Flush.
For other routes... we could make the argument that OrderingBarrier() only
orders wrt other command buffers, and that other command buffers need to
explicitly shallow-synchronize other command buffers to their command buffers
before synchronizing wrt IPCs. So maybe we could make it so that only
ShallowFlush needs to InternalFlush - but in truth any other GL command that
needs to synchronize the command buffer wrt IPCs will issue a ShallowFlush
anyway, so you wouldn't gain anything in practice. Unless we break the
OrderingBarrier() in "half", as an Acquire vs Release barrier. Or explicit
dependencies (shallow sync points). Those become more complex on the client
code.

[vmiura, 2015-02-05 02:34:13]

> > I think doing InternalFlush on every Send may be overkill.  Certain IPCs
must
> be
> > ordered relative to the pending Flush(), but I'm not sure which.
> > 
> > For one, would it be sufficient to run InternalFlush() when the Send is from
> the
> > same route_id as the pending Flush?
> > 
> > Then, are there only specific IPCs within the route that need strict
ordering
> > with the Flush?
> 
> Since InternalFlush is cheap in the common case, maybe it's ok?
> Weakening IPC ordering is kind of scary, we need to be very careful. Most IPCs
> if not all on the same route definitely need ordering wrt Flush.
> For other routes... we could make the argument that OrderingBarrier() only
> orders wrt other command buffers, and that other command buffers need to
> explicitly shallow-synchronize other command buffers to their command buffers
> before synchronizing wrt IPCs. So maybe we could make it so that only
> ShallowFlush needs to InternalFlush - but in truth any other GL command that
> needs to synchronize the command buffer wrt IPCs will issue a ShallowFlush
> anyway, so you wouldn't gain anything in practice. Unless we break the
> OrderingBarrier() in "half", as an Acquire vs Release barrier. Or explicit
> dependencies (shallow sync points). Those become more complex on the client
> code.

I'm going to be calling this OrderingBarrier() on the Ganesh context after each
tile, with the idea that these will mostly be flushed once/twice per frame by
the Compositor context itself doing an OrderingBarrier or Flush.  If any IPC on
the Compositor's context also triggers InternalFlush then it could add up to
more.

Actually thinking about it, would any IPC on the same route need to do the
InternalFlush for itself?  The OrderingBarrier is not something needed
structurally for a single route I think.

For crossing routes, if we could require explicit barriers:
 C1(OrderingBarrier) -> C2(OrderingBarrier) -> other IPC

To be more optimized we could maybe have two forms:
 C1(OrderingFlush) -> C2(OrderingBarrier) -> other IPC, where OrderingBarrier
does an InternalFlush but does not itself queue a flush.

[vmiura, 2015-02-05 19:48:17]

New patchsets have been uploaded after l-g-t-m from jbauman@chromium.org

[vmiura, 2015-02-05 20:06:48]

PTAL.

I changed the name to OrderingBarrierCHROMIUM().

I optimized the case so we don't InternalFlush() when getting a Send() from the
same route_id, as I think this is easy to reason about.  Structural flushes on
the same route require an explicit Flush().  Other optimizations I'll leave for
a later optimization if needed.

[vmiura, 2015-02-06 00:05:55]

Please take another look.

After discussing offline with piman@ it looks good to remove implicit
InternalFlush() inside IPC Send, but CommandBufferHelper &
CommandBufferProxyImpl must not filter Flush & OrderingBarrier themselves.  They
must always be passed down to GpuChannelHost which checks if it needs to run an
InternalFlush().

[piman, 2015-02-06 03:37:48]

LGTM + nits.

https://codereview.chromium.org/896723008/diff/40001/gpu/command_buffer/commo...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/896723008/diff/40001/gpu/command_buffer/commo...
gpu/command_buffer/common/command_buffer.h:93: // As Flush, ensuress that on the
service side, commands up to put_offset
On 2015/02/05 01:14:15, piman (Very slow to review) wrote:
> typo: ensuress

Still here in PS#7...

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

https://codereview.chromium.org/896723008/diff/140001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:186: if (channel_)
nit: style requires brackets because body is >1 line

https://codereview.chromium.org/896723008/diff/140001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:204: if (channel_)
nit: brackets here too.

[vmiura, 2015-02-06 05:15:00]

New patchsets have been uploaded after l-g-t-m from piman@chromium.org

[vmiura, 2015-02-06 05:15:22]

Fixed nits.  Thanks.

https://codereview.chromium.org/896723008/diff/40001/gpu/command_buffer/commo...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/896723008/diff/40001/gpu/command_buffer/commo...
gpu/command_buffer/common/command_buffer.h:93: // As Flush, ensuress that on the
service side, commands up to put_offset
On 2015/02/06 03:37:48, piman (Very slow to review) wrote:
> On 2015/02/05 01:14:15, piman (Very slow to review) wrote:
> > typo: ensuress
> 
> Still here in PS#7...

Done.

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

https://codereview.chromium.org/896723008/diff/140001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:186: if (channel_)
On 2015/02/06 03:37:48, piman (Very slow to review) wrote:
> nit: style requires brackets because body is >1 line

Done.

https://codereview.chromium.org/896723008/diff/140001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:204: if (channel_)
On 2015/02/06 03:37:48, piman (Very slow to review) wrote:
> nit: brackets here too.

Done.

[vmiura, 2015-02-06 05:58:05]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-06 05:58:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896723008/150001

[commit-bot: I haz the power, 2015-02-06 07:15:17]

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

[commit-bot: I haz the power, 2015-02-06 07:15:18]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vmiura, 2015-02-06 16:41:17]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-06 16:42:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896723008/150001

[commit-bot: I haz the power, 2015-02-06 16:43:01]

Committed patchset #8 (id:150001)

[commit-bot: I haz the power, 2015-02-06 16:43:36]

Patchset 8 (id:??) landed as
https://crrev.com/b700b43d82aee159ade4db622dea6bdd920a7d7b
Cr-Commit-Position: refs/heads/master@{#315048}