Added global order numbers to in process command buffers.

Added global order numbers to in process command buffers.

The global order numbers have been generalized and managed by the
sync point manager. The GpuChannel previously managed the global
order numbers by itself, but these order numbers have to be
ordered with respect to order numbers for in process command
buffers as well.

The global order numbers have been merged to a sync point state
class (SyncPointClientState), and later wait/release functions
will be implemented in SyncPointClient.

R=piman@chromium.org, sievers@chromium.org
BUG=514815
Committed: https://crrev.com/a6b0d39acbecdc8585ea240992f933f131ff0bde
Cr-Commit-Position: refs/heads/master@{#350247}

[David Yen, 2015-09-14 23:23:28]

Note that I decided to modify GpuChannel.cc/.h later to help sunnyps@ not have
to deal with such big changes. I will modify it after his CL lands:

https://codereview.chromium.org/1336623004/

[piman, 2015-09-14 23:43:53]

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.cc:41:
SyncPointManager::~SyncPointManager() {}
Can you DCHECK that all client maps are empty? Otherwise it indicates that there
is still a live SyncPointClient that has a (now stale) reference to this.

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.h:59: // Sync Point Manager is
guaranteed to outlive the sync point client.
This seems hard to guarantee if SyncPointClient is RefCountedThreadSafe. E.g. a
PostTask could keep it alive for an arbitrary long time.

I think it may be simpler to have an explicit create/destroy (or
register/unregister)  to manage the client map... I think you only need the
RefCountedThreadSafe property for accesses to processed_order_num_, right? For
the map entry, the owner knows the exact lifetime.

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.h:130: base::Lock
client_maps_lock_[NUM_SYNC_POINT_NAMESPACES];
nit: it seems slightly overkill to have separate locks, given that the locked
operations are fairly rare, so you will have little contention - if any.

[sunnyps, 2015-09-15 00:21:52]

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

[sunnyps, 2015-09-15 00:21:53]

Can you please make SyncPointClient an abstract class and have
GpuCommandBufferStub / InProcessCommandBuffer implement it?

The reason is that they have different semantics for how the order numbers are
accessed. For GpuCommandBufferStub, we lookup the stream the stub belongs to and
use that to lookup the order number. For InProcessCommandBuffer the order
numbers should be a per-thread property. We could register/unregister the stub /
in-process command buffer with the sync point manager thus eliminating the need
for the refcounting of sync point client.

[David Yen, 2015-09-15 17:44:06]

On 2015/09/15 00:21:53, sunnyps wrote:
> Can you please make SyncPointClient an abstract class and have
> GpuCommandBufferStub / InProcessCommandBuffer implement it?
> 
> The reason is that they have different semantics for how the order numbers are
> accessed. For GpuCommandBufferStub, we lookup the stream the stub belongs to
and
> use that to lookup the order number. For InProcessCommandBuffer the order
> numbers should be a per-thread property. We could register/unregister the stub
/
> in-process command buffer with the sync point manager thus eliminating the
need
> for the refcounting of sync point client.

Hmm, the point of this CL was to make the lookup be arbitrary. You assign a
unique "command_buffer_id" and use that to lookup a flat structure
(SyncPointClient) using the command buffer id with the sync point manager. The
command buffer id is a 64 bit number that is namespaced for both the GpuIO case
and the InProcess case (see SyncPointNamespace) so that they do not conflict. In
the InProcess case, the ID is simply an atomically increasing number. For the
GpuIO case it will be the GpuChannelHost process ID (32 bits) and the Route ID
(32 bits). Each stream has it's own route ID right? That should guarantee
uniqueness. Does that sound like it would work for both cases?

This way, when you need to look up order numbers you no longer need to worry
what kind of command buffer it is, you just look up the SyncPointClient and it
has all the information. In terms of integrating fence sync client it is just a
matter of calling GenerateUnprocessedOrderNumber(), BeginProcessingOrderNumber()
and FinishProcessingOrderNumber() at the right locations. Once your CL goes in,
it should be straight forward to place those 3 calls at the right place.

[David Yen, 2015-09-15 18:09:31]

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.cc:41:
SyncPointManager::~SyncPointManager() {}
On 2015/09/14 23:43:53, piman (slow to review) wrote:
> Can you DCHECK that all client maps are empty? Otherwise it indicates that
there
> is still a live SyncPointClient that has a (now stale) reference to this.

I was going to do this, must have forgotten. Done now.

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.h:59: // Sync Point Manager is
guaranteed to outlive the sync point client.
On 2015/09/14 23:43:53, piman (slow to review) wrote:
> This seems hard to guarantee if SyncPointClient is RefCountedThreadSafe. E.g.
a
> PostTask could keep it alive for an arbitrary long time.
> 
> I think it may be simpler to have an explicit create/destroy (or
> register/unregister)  to manage the client map... I think you only need the
> RefCountedThreadSafe property for accesses to processed_order_num_, right? For
> the map entry, the owner knows the exact lifetime.

Hmm, later functions probably do need a pointer to the sync point manager. For
example we will probably need the sync point clients to handle the waiting since
it needs to handle the condition variable as the current sync point
implementation does.

I also remembered I forgot to add in the "unprocessed order number", I made it
so generate order number must be called from the client and that will both
generate a new order number and assign it as an "unprocessed" one.

Calls that use the sync_point_manager_ pointer are all from the command buffer
that is in charge of destroying it though, so it should be safe for the command
buffer to manage when it can call these functions. I'm not sure what the best
way to validate/DCHECK that condition though, I have put in a DCHECK on
sync_point_manager_ now but technically that can be racey since it can be
destroyed on another thread.

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.h:130: base::Lock
client_maps_lock_[NUM_SYNC_POINT_NAMESPACES];
On 2015/09/14 23:43:53, piman (slow to review) wrote:
> nit: it seems slightly overkill to have separate locks, given that the locked
> operations are fairly rare, so you will have little contention - if any.

Done.

[David Yen, 2015-09-15 18:42:45]

On 2015/09/15 17:44:06, David Yen wrote:
> On 2015/09/15 00:21:53, sunnyps wrote:
> > Can you please make SyncPointClient an abstract class and have
> > GpuCommandBufferStub / InProcessCommandBuffer implement it?
> > 
> > The reason is that they have different semantics for how the order numbers
are
> > accessed. For GpuCommandBufferStub, we lookup the stream the stub belongs to
> and
> > use that to lookup the order number. For InProcessCommandBuffer the order
> > numbers should be a per-thread property. We could register/unregister the
stub
> /
> > in-process command buffer with the sync point manager thus eliminating the
> need
> > for the refcounting of sync point client.
> 
> Hmm, the point of this CL was to make the lookup be arbitrary. You assign a
> unique "command_buffer_id" and use that to lookup a flat structure
> (SyncPointClient) using the command buffer id with the sync point manager. The
> command buffer id is a 64 bit number that is namespaced for both the GpuIO
case
> and the InProcess case (see SyncPointNamespace) so that they do not conflict.
In
> the InProcess case, the ID is simply an atomically increasing number. For the
> GpuIO case it will be the GpuChannelHost process ID (32 bits) and the Route ID
> (32 bits). Each stream has it's own route ID right? That should guarantee
> uniqueness. Does that sound like it would work for both cases?
> 
> This way, when you need to look up order numbers you no longer need to worry
> what kind of command buffer it is, you just look up the SyncPointClient and it
> has all the information. In terms of integrating fence sync client it is just
a
> matter of calling GenerateUnprocessedOrderNumber(),
BeginProcessingOrderNumber()
> and FinishProcessingOrderNumber() at the right locations. Once your CL goes
in,
> it should be straight forward to place those 3 calls at the right place.

Sorry, I should have said all streams contain unique route ID's so gpu channel
ID (process ID) + the Route ID should be unique for each route. Each route would
then have a SyncPointClient and we just update the order number along each route
accordingly.

[piman, 2015-09-15 23:47:16]

I think this can work as is, so LGTM+nits, but see comments if you want to make
it even safer.

I have some concerns that the atomic variables are not going to be enough (when
introducing a condition variable), and you'll need a lock (which shouldn't be
contended, so it's ok), but it's hard to tell until we see the whole picture.

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.h:59: // Sync Point Manager is
guaranteed to outlive the sync point client.
On 2015/09/15 18:09:31, David Yen wrote:
> On 2015/09/14 23:43:53, piman (slow to review) wrote:
> > This seems hard to guarantee if SyncPointClient is RefCountedThreadSafe.
E.g.
> a
> > PostTask could keep it alive for an arbitrary long time.
> > 
> > I think it may be simpler to have an explicit create/destroy (or
> > register/unregister)  to manage the client map... I think you only need the
> > RefCountedThreadSafe property for accesses to processed_order_num_, right?
For
> > the map entry, the owner knows the exact lifetime.
> 
> Hmm, later functions probably do need a pointer to the sync point manager. For
> example we will probably need the sync point clients to handle the waiting
since
> it needs to handle the condition variable as the current sync point
> implementation does.
> 
> I also remembered I forgot to add in the "unprocessed order number", I made it
> so generate order number must be called from the client and that will both
> generate a new order number and assign it as an "unprocessed" one.
> 
> Calls that use the sync_point_manager_ pointer are all from the command buffer
> that is in charge of destroying it though, so it should be safe for the
command
> buffer to manage when it can call these functions. I'm not sure what the best
> way to validate/DCHECK that condition though, I have put in a DCHECK on
> sync_point_manager_ now but technically that can be racey since it can be
> destroyed on another thread.

One way to enforce proper behavior is to split the class in 2 parts, separating
the state that is shared across threads
(processed_order_num_/unprocessed_order_num_) from the non-shared state
(everything else). The shared state would stay in a RefCountedThreadSafe class,
but the rest would be in a single-ownership class, that keeps a reference to the
shared state.
E.g. SyncPointClient (the single-ownership part) owns a SyncPointClientState
(the RefCountedThreadSafe shared part). Instead of
SyncPointManager::GetSyncPointClient, you'd have a GetSyncPointClientState that
would (under lock) lookup the client and return its shared state.

https://codereview.chromium.org/1339203002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://codereview.chromium.org/1339203002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sync_point_manager.cc:33:
processing_thread_checker_.DetachFromThread();
Do you need this? I would think the same thread that creates the SyncPointClient
is also the one that processes the messages and will destroy.

https://codereview.chromium.org/1339203002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sync_point_manager.cc:84: it->second->Destroy();
nit: no need to do this under the lock. you can just call client->Destroy()
before taking the lock.

[David Yen, 2015-09-18 18:43:59]

Back to this CL again, I've merged the CommandBufferNamespace changes and
separated out SyncPointClientState. PTAL.

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/sync_point_manager.h:59: // Sync Point Manager is
guaranteed to outlive the sync point client.
On 2015/09/15 23:47:15, piman (slow to review) wrote:
> On 2015/09/15 18:09:31, David Yen wrote:
> > On 2015/09/14 23:43:53, piman (slow to review) wrote:
> > > This seems hard to guarantee if SyncPointClient is RefCountedThreadSafe.
> E.g.
> > a
> > > PostTask could keep it alive for an arbitrary long time.
> > > 
> > > I think it may be simpler to have an explicit create/destroy (or
> > > register/unregister)  to manage the client map... I think you only need
the
> > > RefCountedThreadSafe property for accesses to processed_order_num_, right?
> For
> > > the map entry, the owner knows the exact lifetime.
> > 
> > Hmm, later functions probably do need a pointer to the sync point manager.
For
> > example we will probably need the sync point clients to handle the waiting
> since
> > it needs to handle the condition variable as the current sync point
> > implementation does.
> > 
> > I also remembered I forgot to add in the "unprocessed order number", I made
it
> > so generate order number must be called from the client and that will both
> > generate a new order number and assign it as an "unprocessed" one.
> > 
> > Calls that use the sync_point_manager_ pointer are all from the command
buffer
> > that is in charge of destroying it though, so it should be safe for the
> command
> > buffer to manage when it can call these functions. I'm not sure what the
best
> > way to validate/DCHECK that condition though, I have put in a DCHECK on
> > sync_point_manager_ now but technically that can be racey since it can be
> > destroyed on another thread.
> 
> One way to enforce proper behavior is to split the class in 2 parts,
separating
> the state that is shared across threads
> (processed_order_num_/unprocessed_order_num_) from the non-shared state
> (everything else). The shared state would stay in a RefCountedThreadSafe
class,
> but the rest would be in a single-ownership class, that keeps a reference to
the
> shared state.
> E.g. SyncPointClient (the single-ownership part) owns a SyncPointClientState
> (the RefCountedThreadSafe shared part). Instead of
> SyncPointManager::GetSyncPointClient, you'd have a GetSyncPointClientState
that
> would (under lock) lookup the client and return its shared state.

Done. Since SyncPointClient is no longer ref counted now, I also reverted back
to the original model where it unregisters itself on destruction.

https://codereview.chromium.org/1339203002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/sync_point_manager.cc (right):

https://codereview.chromium.org/1339203002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sync_point_manager.cc:33:
processing_thread_checker_.DetachFromThread();
On 2015/09/15 23:47:15, piman (slow to review) wrote:
> Do you need this? I would think the same thread that creates the
SyncPointClient
> is also the one that processes the messages and will destroy.

Removed.

https://codereview.chromium.org/1339203002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sync_point_manager.cc:84: it->second->Destroy();
On 2015/09/15 23:47:15, piman (slow to review) wrote:
> nit: no need to do this under the lock. you can just call client->Destroy()
> before taking the lock.

Done.

[piman, 2015-09-18 21:39:15]

lgtm

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:503:
DCHECK(context_lost_ || put_offset == state_after_last_flush_.get_offset);
I don't think this DCHECK is true, if the command buffer had to unschedule, it
will return from Flush with some commands left.

OTOH, InProcessCommandBuffer doesn't register a SchedulingChangedCallback on the
GpuScheduler, so if that happens, there won't really be anything to make
progress.

Maybe leave a TODO to explain?

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:97: protected:
nit: order is public then protected then private. DISALLOW_COPY_AND_ASSIGN must
be in the private section.

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:160: uint32_t
GenerateOrderNumber();
I assume you will use this in GpuChannel as well, in a follow-up?

[David Yen, 2015-09-18 22:39:59]

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:503:
DCHECK(context_lost_ || put_offset == state_after_last_flush_.get_offset);
On 2015/09/18 21:39:15, piman (slow to review) wrote:
> I don't think this DCHECK is true, if the command buffer had to unschedule, it
> will return from Flush with some commands left.
> 
> OTOH, InProcessCommandBuffer doesn't register a SchedulingChangedCallback on
the
> GpuScheduler, so if that happens, there won't really be anything to make
> progress.
> 
> Maybe leave a TODO to explain?

Yeah, at least currently it can't happen. If it does in the future, we would
also need to back off on calling "FinishProcessingOrderNumber" until the message
is rescheduled and processed. I can add a comment about this.

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:97: protected:
On 2015/09/18 21:39:15, piman (slow to review) wrote:
> nit: order is public then protected then private. DISALLOW_COPY_AND_ASSIGN
must
> be in the private section.

Done, just placed everything in private.

https://codereview.chromium.org/1339203002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:160: uint32_t
GenerateOrderNumber();
On 2015/09/18 21:39:15, piman (slow to review) wrote:
> I assume you will use this in GpuChannel as well, in a follow-up?

Yes, I considered putting it in here since sunnyps@ already landed his refactor
CL, but I figured you already reviewed this so I'll put it in later.

[David Yen, 2015-09-18 22:59:04]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-09-18 22:59:04]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1339203002/#ps160001
(title: "Added comment about in process descheduling, removed protected section
in SyncPointClient")

[commit-bot: I haz the power, 2015-09-18 22:59:50]

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

[David Yen, 2015-09-19 00:00:54]

The CQ bit was unchecked by dyen@chromium.org

[David Yen, 2015-09-19 00:08:15]

On 2015/09/18 22:59:50, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1339203002/160001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1339203002/160001

Hmm, it seems I've made a mistake here. In our current GpuChannel global order
implementation, we are keeping track of the order at the "channel" level,
whereas this CL makes sync point clients to be at the "route" level. That is
possible by placing them at the GpuCommandBufferStub level, but that would also
mean we have to move the GpuCommandBufferStub map to the GpuChannelMessageQueue
which isn't too bad but we would then need to guard it with a lock...

Keeping track of processed order numbers per route is actually better in the
cases of a bad renderer but that is not the usual case.

So I see 2 options:

1. keep this CL as is and move the stubs_ map to GpuChannelMessageQueue guarded
by a lock.

2. Change this CL so order numbers are some other client that is at the "channel
level" (InProcess channel ID would be the same as the command buffer id),
whereas fence sync functions would be a separate client at the "route level".
That would add a bit more complexity, but not too much. In that case the order
number client doesn't really make sense to be called "SyncPointClient" since at
that point it's not really sync point specific. Global order manager/client or
something?

Does anyone have an opinion on either solutions?

[piman, 2015-09-19 00:38:23]

On Fri, Sep 18, 2015 at 5:08 PM, <dyen@chromium.org> wrote:

> On 2015/09/18 22:59:50, commit-bot: I haz the power wrote:
>
>> CQ is trying da patch. Follow status at
>>   https://chromium-cq-status.appspot.com/patch-status/1339203002/160001
>> View timeline at
>>   https://chromium-cq-status.appspot.com/patch-timeline/1339203002/160001
>>
>
> Hmm, it seems I've made a mistake here. In our current GpuChannel global
> order
> implementation, we are keeping track of the order at the "channel" level,
> whereas this CL makes sync point clients to be at the "route" level. That
> is
> possible by placing them at the GpuCommandBufferStub level, but that would
> also
> mean we have to move the GpuCommandBufferStub map to the
> GpuChannelMessageQueue
> which isn't too bad but we would then need to guard it with a lock...
>
> Keeping track of processed order numbers per route is actually better in
> the
> cases of a bad renderer but that is not the usual case.
>
> So I see 2 options:
>
> 1. keep this CL as is and move the stubs_ map to GpuChannelMessageQueue
> guarded
> by a lock.
>
> 2. Change this CL so order numbers are some other client that is at the
> "channel
> level" (InProcess channel ID would be the same as the command buffer id),
> whereas fence sync functions would be a separate client at the "route
> level".
> That would add a bit more complexity, but not too much. In that case the
> order
> number client doesn't really make sense to be called "SyncPointClient"
> since at
> that point it's not really sync point specific. Global order
> manager/client or
> something?
>
> Does anyone have an opinion on either solutions?
>

processed_order_num_/unprocessed_order_num_ is really at stream
level. current_order_num_ doesn't really matter, it's just used for
context, so that you can get to in in WaitSyncPoint. It could really be
stub/stream/channel or thread-local.

My suggestion: make SyncPointClientState per-GpuChannelMessageQueue (which
is per-channel now, but would be per-stream at some point), and
have SyncPointClient take a reference to it instead of creating it. You
could then still have a SyncPointClient per-stub, and register/unregister
the same way so that you can map the sync point to the
right SyncPointClientState.


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

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

[David Yen, 2015-09-19 00:48:09]

On 2015/09/19 00:38:23, piman (slow to review) wrote:
> On Fri, Sep 18, 2015 at 5:08 PM, <mailto:dyen@chromium.org> wrote:
> 
> > On 2015/09/18 22:59:50, commit-bot: I haz the power wrote:
> >
> >> CQ is trying da patch. Follow status at
> >>   https://chromium-cq-status.appspot.com/patch-status/1339203002/160001
> >> View timeline at
> >>   https://chromium-cq-status.appspot.com/patch-timeline/1339203002/160001
> >>
> >
> > Hmm, it seems I've made a mistake here. In our current GpuChannel global
> > order
> > implementation, we are keeping track of the order at the "channel" level,
> > whereas this CL makes sync point clients to be at the "route" level. That
> > is
> > possible by placing them at the GpuCommandBufferStub level, but that would
> > also
> > mean we have to move the GpuCommandBufferStub map to the
> > GpuChannelMessageQueue
> > which isn't too bad but we would then need to guard it with a lock...
> >
> > Keeping track of processed order numbers per route is actually better in
> > the
> > cases of a bad renderer but that is not the usual case.
> >
> > So I see 2 options:
> >
> > 1. keep this CL as is and move the stubs_ map to GpuChannelMessageQueue
> > guarded
> > by a lock.
> >
> > 2. Change this CL so order numbers are some other client that is at the
> > "channel
> > level" (InProcess channel ID would be the same as the command buffer id),
> > whereas fence sync functions would be a separate client at the "route
> > level".
> > That would add a bit more complexity, but not too much. In that case the
> > order
> > number client doesn't really make sense to be called "SyncPointClient"
> > since at
> > that point it's not really sync point specific. Global order
> > manager/client or
> > something?
> >
> > Does anyone have an opinion on either solutions?
> >
> 
> processed_order_num_/unprocessed_order_num_ is really at stream
> level. current_order_num_ doesn't really matter, it's just used for
> context, so that you can get to in in WaitSyncPoint. It could really be
> stub/stream/channel or thread-local.
> 
> My suggestion: make SyncPointClientState per-GpuChannelMessageQueue (which
> is per-channel now, but would be per-stream at some point), and
> have SyncPointClient take a reference to it instead of creating it. You
> could then still have a SyncPointClient per-stub, and register/unregister
> the same way so that you can map the sync point to the
> right SyncPointClientState.
> 
> 
> >
> > https://codereview.chromium.org/1339203002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

That could work but we would need another level that is not ref counted which
holds the pointer to the sync point manager.

[piman, 2015-09-19 01:01:40]

On Fri, Sep 18, 2015 at 5:48 PM, <dyen@chromium.org> wrote:

> On 2015/09/19 00:38:23, piman (slow to review) wrote:
>
> On Fri, Sep 18, 2015 at 5:08 PM, <mailto:dyen@chromium.org> wrote:
>>
>
> > On 2015/09/18 22:59:50, commit-bot: I haz the power wrote:
>> >
>> >> CQ is trying da patch. Follow status at
>> >>
>> https://chromium-cq-status.appspot.com/patch-status/1339203002/160001
>> >> View timeline at
>> >>
>> https://chromium-cq-status.appspot.com/patch-timeline/1339203002/160001
>> >>
>> >
>> > Hmm, it seems I've made a mistake here. In our current GpuChannel global
>> > order
>> > implementation, we are keeping track of the order at the "channel"
>> level,
>> > whereas this CL makes sync point clients to be at the "route" level.
>> That
>> > is
>> > possible by placing them at the GpuCommandBufferStub level, but that
>> would
>> > also
>> > mean we have to move the GpuCommandBufferStub map to the
>> > GpuChannelMessageQueue
>> > which isn't too bad but we would then need to guard it with a lock...
>> >
>> > Keeping track of processed order numbers per route is actually better in
>> > the
>> > cases of a bad renderer but that is not the usual case.
>> >
>> > So I see 2 options:
>> >
>> > 1. keep this CL as is and move the stubs_ map to GpuChannelMessageQueue
>> > guarded
>> > by a lock.
>> >
>> > 2. Change this CL so order numbers are some other client that is at the
>> > "channel
>> > level" (InProcess channel ID would be the same as the command buffer
>> id),
>> > whereas fence sync functions would be a separate client at the "route
>> > level".
>> > That would add a bit more complexity, but not too much. In that case the
>> > order
>> > number client doesn't really make sense to be called "SyncPointClient"
>> > since at
>> > that point it's not really sync point specific. Global order
>> > manager/client or
>> > something?
>> >
>> > Does anyone have an opinion on either solutions?
>> >
>>
>
> processed_order_num_/unprocessed_order_num_ is really at stream
>> level. current_order_num_ doesn't really matter, it's just used for
>> context, so that you can get to in in WaitSyncPoint. It could really be
>> stub/stream/channel or thread-local.
>>
>
> My suggestion: make SyncPointClientState per-GpuChannelMessageQueue (which
>> is per-channel now, but would be per-stream at some point), and
>> have SyncPointClient take a reference to it instead of creating it. You
>> could then still have a SyncPointClient per-stub, and register/unregister
>> the same way so that you can map the sync point to the
>> right SyncPointClientState.
>>
>
>
> >
>> > https://codereview.chromium.org/1339203002/
>> >
>>
>
> To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> That could work but we would need another level that is not ref counted
> which
> holds the pointer to the sync point manager.
>

The SyncPointManager is guaranteed to outlive the GpuChannelManager (hence
the GpuChannel), as well as the IO thread (hence the filter and the queue),
so that part is ok.


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

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

[David Yen, 2015-09-21 19:17:26]

On 2015/09/19 01:01:40, piman (slow to review) wrote:
> On Fri, Sep 18, 2015 at 5:48 PM, <mailto:dyen@chromium.org> wrote:
> 
> > On 2015/09/19 00:38:23, piman (slow to review) wrote:
> >
> > On Fri, Sep 18, 2015 at 5:08 PM, <mailto:dyen@chromium.org> wrote:
> >>
> >
> > > On 2015/09/18 22:59:50, commit-bot: I haz the power wrote:
> >> >
> >> >> CQ is trying da patch. Follow status at
> >> >>
> >> https://chromium-cq-status.appspot.com/patch-status/1339203002/160001
> >> >> View timeline at
> >> >>
> >> https://chromium-cq-status.appspot.com/patch-timeline/1339203002/160001
> >> >>
> >> >
> >> > Hmm, it seems I've made a mistake here. In our current GpuChannel global
> >> > order
> >> > implementation, we are keeping track of the order at the "channel"
> >> level,
> >> > whereas this CL makes sync point clients to be at the "route" level.
> >> That
> >> > is
> >> > possible by placing them at the GpuCommandBufferStub level, but that
> >> would
> >> > also
> >> > mean we have to move the GpuCommandBufferStub map to the
> >> > GpuChannelMessageQueue
> >> > which isn't too bad but we would then need to guard it with a lock...
> >> >
> >> > Keeping track of processed order numbers per route is actually better in
> >> > the
> >> > cases of a bad renderer but that is not the usual case.
> >> >
> >> > So I see 2 options:
> >> >
> >> > 1. keep this CL as is and move the stubs_ map to GpuChannelMessageQueue
> >> > guarded
> >> > by a lock.
> >> >
> >> > 2. Change this CL so order numbers are some other client that is at the
> >> > "channel
> >> > level" (InProcess channel ID would be the same as the command buffer
> >> id),
> >> > whereas fence sync functions would be a separate client at the "route
> >> > level".
> >> > That would add a bit more complexity, but not too much. In that case the
> >> > order
> >> > number client doesn't really make sense to be called "SyncPointClient"
> >> > since at
> >> > that point it's not really sync point specific. Global order
> >> > manager/client or
> >> > something?
> >> >
> >> > Does anyone have an opinion on either solutions?
> >> >
> >>
> >
> > processed_order_num_/unprocessed_order_num_ is really at stream
> >> level. current_order_num_ doesn't really matter, it's just used for
> >> context, so that you can get to in in WaitSyncPoint. It could really be
> >> stub/stream/channel or thread-local.
> >>
> >
> > My suggestion: make SyncPointClientState per-GpuChannelMessageQueue (which
> >> is per-channel now, but would be per-stream at some point), and
> >> have SyncPointClient take a reference to it instead of creating it. You
> >> could then still have a SyncPointClient per-stub, and register/unregister
> >> the same way so that you can map the sync point to the
> >> right SyncPointClientState.
> >>
> >
> >
> > >
> >> > https://codereview.chromium.org/1339203002/
> >> >
> >>
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > That could work but we would need another level that is not ref counted
> > which
> > holds the pointer to the sync point manager.
> >
> 
> The SyncPointManager is guaranteed to outlive the GpuChannelManager (hence
> the GpuChannel), as well as the IO thread (hence the filter and the queue),
> so that part is ok.
> 
> 
> >
> > https://codereview.chromium.org/1339203002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I just made it so the function that needs SyncPointManager requires it to be
passed in, so I can store all global order numbers in the ref counted
SyncPointClientState. Since sunnyps@ landed his patch, I also placed the calls
needed to support SyncPointClient in GpuChannel/GpuCommandBufferStub. PTAL.

[sunnyps, 2015-09-22 17:26:00]

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:154:
sync_point_client_state_->BeginProcessingOrderNumber(m->order_number);
How does this work? GetNextMessage is const but BeginProcessingOrderNumber is
non-const right?

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:413: scoped_refptr<gpu::SyncPointClientState>
sync_point_client_state();
snake case naming convention is for inline accessors only

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:450: void
PushMessageHelper(gpu::SyncPointManager* sync_point_manager,
Maybe initialize the message queue with a sync point manager?

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:28: : public
base::RefCountedThreadSafe<SyncPointClientState> {
Why is SyncPointClientState ref counted thread safe? It looks like it's
guaranteed to outlive the command buffer and sync point client.

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:88: friend class
SyncPointManager;
Why do we need friend access here?

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:145: friend class
SyncPointClientState;
Why do we need friend access here? Only for GenerateOrderNumber?

[piman, 2015-09-22 19:43:09]

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:107: if (enabled_)
nit: needs {} per style.

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:154:
sync_point_client_state_->BeginProcessingOrderNumber(m->order_number);
On 2015/09/22 17:25:59, sunnyps wrote:
> How does this work? GetNextMessage is const but BeginProcessingOrderNumber is
> non-const right?

scoped_refptr::operator-> is const but returns a T*.

But I agree with the semantic meaning that either GetNextMessage should be
non-const, or the caller should be the one calling BeginProcessingOrderNumber

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:28: : public
base::RefCountedThreadSafe<SyncPointClientState> {
On 2015/09/22 17:25:59, sunnyps wrote:
> Why is SyncPointClientState ref counted thread safe? It looks like it's
> guaranteed to outlive the command buffer and sync point client.

It's not, because GetSyncPointClientState can be called by another command
buffer on another thread.

[David Yen, 2015-09-22 20:01:54]

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:107: if (enabled_)
On 2015/09/22 19:43:08, piman (slow to review) wrote:
> nit: needs {} per style.

Done.

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:154:
sync_point_client_state_->BeginProcessingOrderNumber(m->order_number);
On 2015/09/22 19:43:08, piman (slow to review) wrote:
> On 2015/09/22 17:25:59, sunnyps wrote:
> > How does this work? GetNextMessage is const but BeginProcessingOrderNumber
is
> > non-const right?
> 
> scoped_refptr::operator-> is const but returns a T*.
> 
> But I agree with the semantic meaning that either GetNextMessage should be
> non-const, or the caller should be the one calling BeginProcessingOrderNumber

Done.

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:413: scoped_refptr<gpu::SyncPointClientState>
sync_point_client_state();
On 2015/09/22 17:25:59, sunnyps wrote:
> snake case naming convention is for inline accessors only

Done.

https://codereview.chromium.org/1339203002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:450: void
PushMessageHelper(gpu::SyncPointManager* sync_point_manager,
On 2015/09/22 17:25:59, sunnyps wrote:
> Maybe initialize the message queue with a sync point manager?

Passing it in seems better here so we don't have to worry about lifetime
maintenance. Either way is fine by me though...

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:28: : public
base::RefCountedThreadSafe<SyncPointClientState> {
On 2015/09/22 17:25:59, sunnyps wrote:
> Why is SyncPointClientState ref counted thread safe? It looks like it's
> guaranteed to outlive the command buffer and sync point client.

It is queriable from the SyncPointManager::GetSyncPointClientState() and could
be used on different threads (InProcess thread).

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:88: friend class
SyncPointManager;
On 2015/09/22 17:25:59, sunnyps wrote:
> Why do we need friend access here

Creation of a SyncPointClient must be done through the SyncPointManager
(SyncPointManager::CreateSyncPointClient).

https://codereview.chromium.org/1339203002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:145: friend class
SyncPointClientState;
On 2015/09/22 17:25:59, sunnyps wrote:
> Why do we need friend access here? Only for GenerateOrderNumber?

Yes, this was to ensure only SyncPointClient can unregister itself, and
SyncPointClientState can generate order numbers.

[piman, 2015-09-22 20:56:22]

lgtm

[David Yen, 2015-09-22 21:02:58]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-09-22 21:06:57]

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

[commit-bot: I haz the power, 2015-09-22 21:53:35]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-09-22 22:09:37]

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