Cleanup GpuMemoryBuffer

We need to cleanup GpuMemoryBuffers allocated in the GPU process.

This code conditionally stashes a sync point for buffer types that require it for the destruction phase.

BUG=416873
Committed: https://crrev.com/2992e56e7d52e3158400980b312ecf0a8ddbfe1d
Cr-Commit-Position: refs/heads/master@{#302561}

[alexst (slow to review), 2014-10-21 17:32:43]

Patchset #1 (id:1) has been deleted

[alexst (slow to review), 2014-10-21 17:32:56]

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

[alexst (slow to review), 2014-10-21 17:36:15]

Hi Dave,

This is drafty and omitting the plumbing from 

GpuMemoryBufferFactoryHostImpl::DestroyGpuMemoryBuffer to 

GpuChannelHost::WaitForPendingGpuMemoryBufferUsageToComplete

for now. This only has the changes that would be needed in GpuChannelHost.

What do you think?

[reveman, 2014-10-21 20:27:58]

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

https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:341:
signal_counters_.insert(std::make_pair(id, num_proxies));
Instead of signal_counters_ and signal_tasks_ maps, could we create some
refcounted object that you pass to SignalSyncPoint and simple post a task to the
IO thread from the dtor of this object? I'm thinking this will make it easy to
ensure that we behave properly when a proxy is removed while we're waiting for
usage to complete. Would also remove the need for hash maps and signal ids.

https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.h:164: // To be called on IO thread.
I assume it's more appropriate to do the thread hop in this implementation than
requiring it to be done by the caller, correct?

[alexst (slow to review), 2014-10-22 14:47:08]

I refactored the code per your comment.

It now allows the wait call to be made on any thread and it will fire the
finished callback on the same thread.

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

https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:341:
signal_counters_.insert(std::make_pair(id, num_proxies));
On 2014/10/21 20:27:58, reveman wrote:
> Instead of signal_counters_ and signal_tasks_ maps, could we create some
> refcounted object that you pass to SignalSyncPoint and simple post a task to
the
> IO thread from the dtor of this object? I'm thinking this will make it easy to
> ensure that we behave properly when a proxy is removed while we're waiting for
> usage to complete. Would also remove the need for hash maps and signal ids.

That's a good idea, it deals with proxy removal cleanly. I don't think we can
get rid of signal_tasks_ though because external callbacks are not supposed to
cross thread boundaries.

https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.h:164: // To be called on IO thread.
On 2014/10/21 20:27:58, reveman wrote:
> I assume it's more appropriate to do the thread hop in this implementation
than
> requiring it to be done by the caller, correct?

I was initially thinking that since the browser side already does this on IO, it
was reasonably to enforce, but it occurs to me that we don't know which thread
might call it in a renderer, so the restriction is probably invalid.

[reveman, 2014-10-22 16:20:09]

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

https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:341:
signal_counters_.insert(std::make_pair(id, num_proxies));
On 2014/10/22 14:47:08, alexst wrote:
> On 2014/10/21 20:27:58, reveman wrote:
> > Instead of signal_counters_ and signal_tasks_ maps, could we create some
> > refcounted object that you pass to SignalSyncPoint and simple post a task to
> the
> > IO thread from the dtor of this object? I'm thinking this will make it easy
to
> > ensure that we behave properly when a proxy is removed while we're waiting
for
> > usage to complete. Would also remove the need for hash maps and signal ids.
> 
> That's a good idea, it deals with proxy removal cleanly. I don't think we can
> get rid of signal_tasks_ though because external callbacks are not supposed to
> cross thread boundaries.

hm, does it matter when you can guaranteed that it's not used or destroyed on
the other thread?

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

https://codereview.chromium.org/654223006/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:373: }
Could we make all this much simpler by just having a
WaitForPendingGpuMemoryBufferUsageToComplete on the proxy class and it would be
responsible for calling it in case it was destroyed?

Then we can simply use a base::BarrierClosure to create a compound callback
here.

[alexst (slow to review), 2014-10-22 16:46:54]

On 2014/10/22 16:20:09, reveman wrote:
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> File content/common/gpu/client/gpu_channel_host.cc (right):
> 
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> content/common/gpu/client/gpu_channel_host.cc:341:
> signal_counters_.insert(std::make_pair(id, num_proxies));
> On 2014/10/22 14:47:08, alexst wrote:
> > On 2014/10/21 20:27:58, reveman wrote:
> > > Instead of signal_counters_ and signal_tasks_ maps, could we create some
> > > refcounted object that you pass to SignalSyncPoint and simple post a task
to
> > the
> > > IO thread from the dtor of this object? I'm thinking this will make it
easy
> to
> > > ensure that we behave properly when a proxy is removed while we're waiting
> for
> > > usage to complete. Would also remove the need for hash maps and signal
ids.
> > 
> > That's a good idea, it deals with proxy removal cleanly. I don't think we
can
> > get rid of signal_tasks_ though because external callbacks are not supposed
to
> > cross thread boundaries.
> 
> hm, does it matter when you can guaranteed that it's not used or destroyed on
> the other thread?

I discovered this subtlety recently when Antoine mentioned it on another CL:

"This pattern, passing a callback to another thread, is dangerous in subtle
ways.

A callback refcounts its storage. Because of this, you can't guarantee which
thread the storage gets destroyed, if you pass the callback to another thread.
Since the callback comes from the user, you don't know if its storage might
contain non-thread-safe references (e.g. a RefCounted class), in which case
destroying the storage on another thread would cause bad things."

[reveman, 2014-10-22 17:03:27]

On 2014/10/22 16:46:54, alexst wrote:
> On 2014/10/22 16:20:09, reveman wrote:
> >
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> > File content/common/gpu/client/gpu_channel_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> > content/common/gpu/client/gpu_channel_host.cc:341:
> > signal_counters_.insert(std::make_pair(id, num_proxies));
> > On 2014/10/22 14:47:08, alexst wrote:
> > > On 2014/10/21 20:27:58, reveman wrote:
> > > > Instead of signal_counters_ and signal_tasks_ maps, could we create some
> > > > refcounted object that you pass to SignalSyncPoint and simple post a
task
> to
> > > the
> > > > IO thread from the dtor of this object? I'm thinking this will make it
> easy
> > to
> > > > ensure that we behave properly when a proxy is removed while we're
waiting
> > for
> > > > usage to complete. Would also remove the need for hash maps and signal
> ids.
> > > 
> > > That's a good idea, it deals with proxy removal cleanly. I don't think we
> can
> > > get rid of signal_tasks_ though because external callbacks are not
supposed
> to
> > > cross thread boundaries.
> > 
> > hm, does it matter when you can guaranteed that it's not used or destroyed
on
> > the other thread?
> 
> I discovered this subtlety recently when Antoine mentioned it on another CL:
> 
> "This pattern, passing a callback to another thread, is dangerous in subtle
> ways.
> 
> A callback refcounts its storage. Because of this, you can't guarantee which
> thread the storage gets destroyed, if you pass the callback to another thread.
> Since the callback comes from the user, you don't know if its storage might
> contain non-thread-safe references (e.g. a RefCounted class), in which case
> destroying the storage on another thread would cause bad things."

In this case you would be able to guarantee which thread the storage gets
destroyed but maybe still a bad idea so let's ignore this for now. On a related
note, why do we have to make a hop to the main thread? What makes the main
thread unique?

[alexst (slow to review), 2014-10-22 17:17:14]

On 2014/10/22 17:03:27, reveman wrote:
> On 2014/10/22 16:46:54, alexst wrote:
> > On 2014/10/22 16:20:09, reveman wrote:
> > >
> >
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> > > File content/common/gpu/client/gpu_channel_host.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> > > content/common/gpu/client/gpu_channel_host.cc:341:
> > > signal_counters_.insert(std::make_pair(id, num_proxies));
> > > On 2014/10/22 14:47:08, alexst wrote:
> > > > On 2014/10/21 20:27:58, reveman wrote:
> > > > > Instead of signal_counters_ and signal_tasks_ maps, could we create
some
> > > > > refcounted object that you pass to SignalSyncPoint and simple post a
> task
> > to
> > > > the
> > > > > IO thread from the dtor of this object? I'm thinking this will make it
> > easy
> > > to
> > > > > ensure that we behave properly when a proxy is removed while we're
> waiting
> > > for
> > > > > usage to complete. Would also remove the need for hash maps and signal
> > ids.
> > > > 
> > > > That's a good idea, it deals with proxy removal cleanly. I don't think
we
> > can
> > > > get rid of signal_tasks_ though because external callbacks are not
> supposed
> > to
> > > > cross thread boundaries.
> > > 
> > > hm, does it matter when you can guaranteed that it's not used or destroyed
> on
> > > the other thread?
> > 
> > I discovered this subtlety recently when Antoine mentioned it on another CL:
> > 
> > "This pattern, passing a callback to another thread, is dangerous in subtle
> > ways.
> > 
> > A callback refcounts its storage. Because of this, you can't guarantee which
> > thread the storage gets destroyed, if you pass the callback to another
thread.
> > Since the callback comes from the user, you don't know if its storage might
> > contain non-thread-safe references (e.g. a RefCounted class), in which case
> > destroying the storage on another thread would cause bad things."
> 
> In this case you would be able to guarantee which thread the storage gets
> destroyed but maybe still a bad idea so let's ignore this for now. On a
related
> note, why do we have to make a hop to the main thread? What makes the main
> thread unique?

Because CommandBufferProxyImpl makes no guarantees about thread safety and as I
understand is used on the main thread. Am I mistaken?

[reveman, 2014-10-22 17:41:39]

On 2014/10/22 17:17:14, alexst wrote:
> On 2014/10/22 17:03:27, reveman wrote:
> > On 2014/10/22 16:46:54, alexst wrote:
> > > On 2014/10/22 16:20:09, reveman wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> > > > File content/common/gpu/client/gpu_channel_host.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/654223006/diff/20001/content/common/gpu/clien...
> > > > content/common/gpu/client/gpu_channel_host.cc:341:
> > > > signal_counters_.insert(std::make_pair(id, num_proxies));
> > > > On 2014/10/22 14:47:08, alexst wrote:
> > > > > On 2014/10/21 20:27:58, reveman wrote:
> > > > > > Instead of signal_counters_ and signal_tasks_ maps, could we create
> some
> > > > > > refcounted object that you pass to SignalSyncPoint and simple post a
> > task
> > > to
> > > > > the
> > > > > > IO thread from the dtor of this object? I'm thinking this will make
it
> > > easy
> > > > to
> > > > > > ensure that we behave properly when a proxy is removed while we're
> > waiting
> > > > for
> > > > > > usage to complete. Would also remove the need for hash maps and
signal
> > > ids.
> > > > > 
> > > > > That's a good idea, it deals with proxy removal cleanly. I don't think
> we
> > > can
> > > > > get rid of signal_tasks_ though because external callbacks are not
> > supposed
> > > to
> > > > > cross thread boundaries.
> > > > 
> > > > hm, does it matter when you can guaranteed that it's not used or
destroyed
> > on
> > > > the other thread?
> > > 
> > > I discovered this subtlety recently when Antoine mentioned it on another
CL:
> > > 
> > > "This pattern, passing a callback to another thread, is dangerous in
subtle
> > > ways.
> > > 
> > > A callback refcounts its storage. Because of this, you can't guarantee
which
> > > thread the storage gets destroyed, if you pass the callback to another
> thread.
> > > Since the callback comes from the user, you don't know if its storage
might
> > > contain non-thread-safe references (e.g. a RefCounted class), in which
case
> > > destroying the storage on another thread would cause bad things."
> > 
> > In this case you would be able to guarantee which thread the storage gets
> > destroyed but maybe still a bad idea so let's ignore this for now. On a
> related
> > note, why do we have to make a hop to the main thread? What makes the main
> > thread unique?
> 
> Because CommandBufferProxyImpl makes no guarantees about thread safety and as
I
> understand is used on the main thread. Am I mistaken?

It can be used on the compositor thread. Probably better to add a
WaitForPendingGpuMemoryBufferUsageToComplete function to the proxy impl that can
be called on any thread and it's up to the implementation to handle that instead
of reaching in and using InsertSyncPoint etc from GpuChannelHost.

[alexst (slow to review), 2014-10-22 20:02:45]

new draft, PTAL.

[alexst (slow to review), 2014-10-22 20:05:01]

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:276:
gpu_memory_buffer_tasks_.insert(std::make_pair(sync_point, callback));
Forgot to add, these are to be called in the destructor. if the proxy goes away.

[reveman, 2014-10-22 22:42:47]

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:174: base::AutoLock
lock(signal_lock_);
Alternatively, we can make sure to only access signal_tasks_ on the thread that
is set up to process messages for this command buffer. Looks like using
MessageLoopProxy::current() at the time the ctor is called would yield the
correct result.

We should probably avoid adding a lock where a lock is currently not needed.

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:276:
gpu_memory_buffer_tasks_.insert(std::make_pair(sync_point, callback));
On 2014/10/22 20:05:01, alexst wrote:
> Forgot to add, these are to be called in the destructor. if the proxy goes
away.

Will the dtor be called on the same thread that called this function?

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.h:181: void
GpuMemoryBufferSyncPointSignalled(uint32 sync_point);
GpuMemoryBufferUsageCompleted?

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.h:221: SignalTaskMap
gpu_memory_buffer_tasks_;
I think we should use a different id namespace and map for these tasks.

GpuMemoryBufferUsageCompletedMap gpu_memory_buffer_usage_completed_tasks_;

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

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:333: base::Unretained(this),
is this safe?

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:342: void
GpuChannelHost::GpuMemoryBufferWaitCompleted(
GpuMemoryBufferUsageCompleted

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:348: base::Unretained(this),
is this safe? what if the GpuChannelHost is deleted while this is pending?

it seems like it would be easier to ensure that
GpuMemoryBufferUsageCompleted/OnCallerThread was always called and the callback
passed to ::WaitForPendingGpuMemoryBufferUsageToComplete was always deleted on
the correct thread if you make GpuMemoryBufferUsageCompleted/OnCallerThread
non-member functions and instead passed ownership of the callback between those
functions.

Using a map to ensure that "usage complete" callbacks are destroyed on the
caller thread only works if GpuChannelHost is guaranteed to be destroyed on the
caller thread. Doesn't look like that's necessarily the case here.

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.h (right):

https://codereview.chromium.org/654223006/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.h:257: typedef base::hash_map<uint32,
base::Closure> SignalTaskMap;
GpuMemoryBufferUsageCompletedTaskMap?

[alexst (slow to review), 2014-10-23 19:22:05]

Okay, should be cleaner now, I moved most of the logic into the proxy. It should
be safe now, doesn't add locks where there weren't any and returns on calling
thread from the proxy.

[reveman, 2014-10-23 19:46:43]

https://codereview.chromium.org/654223006/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/654223006/diff/80001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.h:167:
scoped_refptr<base::MessageLoopProxy> message_loop;
I just realized that GpuChannelHost::MessageFilter already handle routing of IPC
messages from the IO thread to the correct proxy thread. Could we remove the
routing here and instead use that? All
Proxy::WaitForPendingGpuMemoryBufferUsageToComplete calls would be guaranteed to
happen on the same thread as other calls.

[alexst (slow to review), 2014-10-23 21:48:47]

On 2014/10/23 19:46:43, reveman wrote:
>
https://codereview.chromium.org/654223006/diff/80001/content/common/gpu/clien...
> File content/common/gpu/client/command_buffer_proxy_impl.h (right):
> 
>
https://codereview.chromium.org/654223006/diff/80001/content/common/gpu/clien...
> content/common/gpu/client/command_buffer_proxy_impl.h:167:
> scoped_refptr<base::MessageLoopProxy> message_loop;
> I just realized that GpuChannelHost::MessageFilter already handle routing of
IPC
> messages from the IO thread to the correct proxy thread. Could we remove the
> routing here and instead use that? All
> Proxy::WaitForPendingGpuMemoryBufferUsageToComplete calls would be guaranteed
to
> happen on the same thread as other calls.

It would probably be more complicated. MessageFilter works on IO, so we either
have to make another thread hop to get the proxy's message loop, use another
lock, or duplicate the message loop cache on the channel host. We also have to
duplicate the logic that stashes the callback on the same thread into the
channel host. Once we make the thread hop in the channel host, we will need to
know the original caller's message loop in the proxy to signal it's destruction
on that thread in case the proxy goes away before the callback triggered. We
will have multithreaded code in two places instead of one.

On the topic of destruction, is it actually safe to say that proxy's destruction
is the same as the sync point being signalled? Is it possible the proxy is
destroyed before sync points are reached? The code in proxy seems to assume that
all pending sync points are signalled before destruction, nothing is done with
signal_tasks_ in the destructor.

[piman, 2014-10-23 23:27:42]

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

[piman, 2014-10-23 23:27:43]

drive-by: I went through this CL, and I'm not sure this is the right approach:

1- doing the Insert/SignalSyncPoint does synchronization with the channel but
not with the command buffers. These may be out-of-order wrt the commands
2- doing an Insert/Signal on each of the CommandBufferProxyImpls is redundant -
they all go through the same channel, and their IPCs are ordered wrt each other.

I'm not fully sure what we're trying to synchronize against any more - it's so
hard to follow with all the changes.
If the only thing we need is to make sure the GpuMsg_DestroyGpuMemoryBuffer
message doesn't overtake the GpuCommandBufferMsg_CreateImage, why don't we do
the InsertSyncPoint at CreateImage time, and stash that into the
GpuMemoryBuffer, so that it can flow back at destruction time?

For that matter, I'm not sure how
CommandBufferProxyImpl::CreateGpuMemoryBufferImage does synchronization today,
even for the renderer case. GpuChannelHost::ShareGpuMemoryBufferToGpuProcess
only dups the handle for shared memory buffers, so can't the Destroy flow
(through the browser) overtake the CreateImage IPC and delete the map entry in
the GPU process before the CreateImage gets handled?

[reveman, 2014-10-24 00:29:45]

On 2014/10/23 21:48:47, alexst wrote:
> On 2014/10/23 19:46:43, reveman wrote:
> >
>
https://codereview.chromium.org/654223006/diff/80001/content/common/gpu/clien...
> > File content/common/gpu/client/command_buffer_proxy_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/654223006/diff/80001/content/common/gpu/clien...
> > content/common/gpu/client/command_buffer_proxy_impl.h:167:
> > scoped_refptr<base::MessageLoopProxy> message_loop;
> > I just realized that GpuChannelHost::MessageFilter already handle routing of
> IPC
> > messages from the IO thread to the correct proxy thread. Could we remove the
> > routing here and instead use that? All
> > Proxy::WaitForPendingGpuMemoryBufferUsageToComplete calls would be
guaranteed
> to
> > happen on the same thread as other calls.
> 
> It would probably be more complicated. MessageFilter works on IO, so we either
> have to make another thread hop to get the proxy's message loop, use another
> lock, or duplicate the message loop cache on the channel host. We also have to
> duplicate the logic that stashes the callback on the same thread into the
> channel host. Once we make the thread hop in the channel host, we will need to
> know the original caller's message loop in the proxy to signal it's
destruction
> on that thread in case the proxy goes away before the callback triggered. We
> will have multithreaded code in two places instead of one.

We wouldn't need the caller message loop in the proxy as all processing as far
as the proxy knows will happen on it's normal thread. Basically, we'd have
multi-thread code in the GpuChannelHost (which we already do) but not have to
deal with threads in the proxy. I really don't think we should be keeping track
of the message loop used for each proxy both in GpuChannelHost and the proxy. I
think it's better to add another thread hop to the IO thread if necessary.

> 
> On the topic of destruction, is it actually safe to say that proxy's
destruction
> is the same as the sync point being signalled? Is it possible the proxy is
> destroyed before sync points are reached? The code in proxy seems to assume
that
> all pending sync points are signalled before destruction, nothing is done with
> signal_tasks_ in the destructor.

Normal sync points should probably not signal as a result of the proxy being
destroyed. They should probably just be removed.

[alexst (slow to review), 2014-10-24 20:19:42]

> I'm not fully sure what we're trying to synchronize against any more - it's so
> hard to follow with all the changes.
> If the only thing we need is to make sure the GpuMsg_DestroyGpuMemoryBuffer
> message doesn't overtake the GpuCommandBufferMsg_CreateImage, why don't we do
> the InsertSyncPoint at CreateImage time, and stash that into the
> GpuMemoryBuffer, so that it can flow back at destruction time?

The issue is still the reface between GpuMsg_DestroyGpuMemoryBuffer and
GpuCommandBufferMsg_CreateImage.

There are two patches up, the one I posted originally that did basically what
you describe, Create, InsertSyncPoint, Stash for destruction, and this one. Dave
expressed a concern about GpuMemoryBuffers registered with multiple command
buffers and how the first patch was going to interact with multiple sync points,
as well as the cost of inserting the sync point when multiple GpuMemoryBuffers
are created in quick succession. After talking offline, Dave asked to see what
it would look like to insert sync points into all proxies and do a client side
wait before destruction as an alternative. This patch is that exploration.

After looking at both patches, I still think it's cleaner to insert the sync
point conditionally after the CreateImage IPC for buffer types that need it, so
long as multiple command buffer problem can be addressed. But maybe we should
all agree on an approach before writing any more code.

[reveman, 2014-10-24 21:33:02]

On 2014/10/23 23:27:43, piman (Very slow to review) wrote:
> drive-by: I went through this CL, and I'm not sure this is the right approach:
> 
> 1- doing the Insert/SignalSyncPoint does synchronization with the channel but
> not with the command buffers. These may be out-of-order wrt the commands

Synchronization with the channel is all we need.

> 2- doing an Insert/Signal on each of the CommandBufferProxyImpls is redundant
-
> they all go through the same channel, and their IPCs are ordered wrt each
other.

Ok, that simplifies things enormously as we don't have to worry about situations
where the last issued CreateImage command for a GpuMemoryBuffer has arrived but
not the second to last.

> 
> I'm not fully sure what we're trying to synchronize against any more - it's so
> hard to follow with all the changes.
> If the only thing we need is to make sure the GpuMsg_DestroyGpuMemoryBuffer
> message doesn't overtake the GpuCommandBufferMsg_CreateImage, why don't we do
> the InsertSyncPoint at CreateImage time, and stash that into the
> GpuMemoryBuffer, so that it can flow back at destruction time?
> 
> For that matter, I'm not sure how
> CommandBufferProxyImpl::CreateGpuMemoryBufferImage does synchronization today,
> even for the renderer case. GpuChannelHost::ShareGpuMemoryBufferToGpuProcess
> only dups the handle for shared memory buffers, so can't the Destroy flow
> (through the browser) overtake the CreateImage IPC and delete the map entry in
> the GPU process before the CreateImage gets handled?

Yes, that's what we're trying to fix here.

If we create a sync point at GpuCommandBufferMsg_CreateImage time, then we don't
have a choice but to do it for each CreateImage IPC. That might be a performance
problem. Ignoring performance problems, the need for sync points is
implementation specific and doesn't belong in the base gfx::GpuMemoryBuffer
class so we'll need some non-trivial plumbing to create and stash sync points
inside GpuMemoryBuffer instances. Maybe that's still the best solution.

GpuChannelHost::ShareGpuMemoryBufferToGpuProcess needs to be moved to
GpuMemoryBufferImpl. That's somewhat related so maybe we can start by doing
that?

[piman, 2014-10-24 22:02:39]

On Fri, Oct 24, 2014 at 2:33 PM, <reveman@chromium.org> wrote:

> On 2014/10/23 23:27:43, piman (Very slow to review) wrote:
>
>> drive-by: I went through this CL, and I'm not sure this is the right
>> approach:
>>
>
>  1- doing the Insert/SignalSyncPoint does synchronization with the channel
>> but
>> not with the command buffers. These may be out-of-order wrt the commands
>>
>
> Synchronization with the channel is all we need.
>
>  2- doing an Insert/Signal on each of the CommandBufferProxyImpls is
>> redundant
>>
> -
>
>> they all go through the same channel, and their IPCs are ordered wrt each
>>
> other.
>
> Ok, that simplifies things enormously as we don't have to worry about
> situations
> where the last issued CreateImage command for a GpuMemoryBuffer has
> arrived but
> not the second to last.
>
>
>  I'm not fully sure what we're trying to synchronize against any more -
>> it's so
>> hard to follow with all the changes.
>> If the only thing we need is to make sure the
>> GpuMsg_DestroyGpuMemoryBuffer
>> message doesn't overtake the GpuCommandBufferMsg_CreateImage, why don't
>> we do
>> the InsertSyncPoint at CreateImage time, and stash that into the
>> GpuMemoryBuffer, so that it can flow back at destruction time?
>>
>
>  For that matter, I'm not sure how
>> CommandBufferProxyImpl::CreateGpuMemoryBufferImage does synchronization
>> today,
>> even for the renderer case. GpuChannelHost::
>> ShareGpuMemoryBufferToGpuProcess
>> only dups the handle for shared memory buffers, so can't the Destroy flow
>> (through the browser) overtake the CreateImage IPC and delete the map
>> entry in
>> the GPU process before the CreateImage gets handled?
>>
>
> Yes, that's what we're trying to fix here.
>
> If we create a sync point at GpuCommandBufferMsg_CreateImage time, then
> we don't
> have a choice but to do it for each CreateImage IPC. That might be a
> performance
> problem. Ignoring performance problems, the need for sync points is
> implementation specific and doesn't belong in the base gfx::GpuMemoryBuffer
> class


GpuMemoryBufferImplSharedMemory is more the exception than the rule. It's
the only one where you can take a reference to the buffer on the client
side without having to round trip to the GPU process, and pass that
reference to the GPU process on the client channel.
We should accommodate the optimization that this allows (no need for sync
points), but I think making the rest of the code more complex because of it
hurts more than helps. So I think adding sync points to GpuMemoryBufferImpl
is not crazy.


> so we'll need some non-trivial plumbing to create and stash sync points
> inside GpuMemoryBuffer instances. Maybe that's still the best solution.
>
> GpuChannelHost::ShareGpuMemoryBufferToGpuProcess needs to be moved to
> GpuMemoryBufferImpl. That's somewhat related so maybe we can start by doing
> that?
>

Possibly... that encodes to some extent the referencing semantics, so it
would be useful to have it all in one place. Though it sounds like we still
need a "before CreateImage" (e.g. dup the fd) and a "after CreateImage"
(e.g. insert and stash sync points) parts.
OTOH, you need the GpuChannelHost to be able to do the
ShareGpuMemoryBufferToGpuProcess (because you need the actual channel to be
able to dup on Windows). You'll also need the channel to make a sync point.
So you'll need to tie the GpuMemoryBufferImpl to the channel (or at least
these 2 operations, may be ok?).


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

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

[reveman, 2014-10-24 22:31:38]

On 2014/10/24 22:02:39, piman (Very slow to review) wrote:
> On Fri, Oct 24, 2014 at 2:33 PM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2014/10/23 23:27:43, piman (Very slow to review) wrote:
> >
> >> drive-by: I went through this CL, and I'm not sure this is the right
> >> approach:
> >>
> >
> >  1- doing the Insert/SignalSyncPoint does synchronization with the channel
> >> but
> >> not with the command buffers. These may be out-of-order wrt the commands
> >>
> >
> > Synchronization with the channel is all we need.
> >
> >  2- doing an Insert/Signal on each of the CommandBufferProxyImpls is
> >> redundant
> >>
> > -
> >
> >> they all go through the same channel, and their IPCs are ordered wrt each
> >>
> > other.
> >
> > Ok, that simplifies things enormously as we don't have to worry about
> > situations
> > where the last issued CreateImage command for a GpuMemoryBuffer has
> > arrived but
> > not the second to last.
> >
> >
> >  I'm not fully sure what we're trying to synchronize against any more -
> >> it's so
> >> hard to follow with all the changes.
> >> If the only thing we need is to make sure the
> >> GpuMsg_DestroyGpuMemoryBuffer
> >> message doesn't overtake the GpuCommandBufferMsg_CreateImage, why don't
> >> we do
> >> the InsertSyncPoint at CreateImage time, and stash that into the
> >> GpuMemoryBuffer, so that it can flow back at destruction time?
> >>
> >
> >  For that matter, I'm not sure how
> >> CommandBufferProxyImpl::CreateGpuMemoryBufferImage does synchronization
> >> today,
> >> even for the renderer case. GpuChannelHost::
> >> ShareGpuMemoryBufferToGpuProcess
> >> only dups the handle for shared memory buffers, so can't the Destroy flow
> >> (through the browser) overtake the CreateImage IPC and delete the map
> >> entry in
> >> the GPU process before the CreateImage gets handled?
> >>
> >
> > Yes, that's what we're trying to fix here.
> >
> > If we create a sync point at GpuCommandBufferMsg_CreateImage time, then
> > we don't
> > have a choice but to do it for each CreateImage IPC. That might be a
> > performance
> > problem. Ignoring performance problems, the need for sync points is
> > implementation specific and doesn't belong in the base gfx::GpuMemoryBuffer
> > class
> 
> 
> GpuMemoryBufferImplSharedMemory is more the exception than the rule. It's
> the only one where you can take a reference to the buffer on the client
> side without having to round trip to the GPU process, and pass that
> reference to the GPU process on the client channel.
> We should accommodate the optimization that this allows (no need for sync
> points), but I think making the rest of the code more complex because of it
> hurts more than helps. So I think adding sync points to GpuMemoryBufferImpl
> is not crazy.

I think we should strive for ownership passing using IPC as that's more
efficient and less complicated to get right, and as a result more secure.

I'd like to see these ozone native buffers pass ownership using IPC in the
future and whatever EGL extension we might propose as the standard way to
implement this should allow that too.

> 
> 
> > so we'll need some non-trivial plumbing to create and stash sync points
> > inside GpuMemoryBuffer instances. Maybe that's still the best solution.
> >
> > GpuChannelHost::ShareGpuMemoryBufferToGpuProcess needs to be moved to
> > GpuMemoryBufferImpl. That's somewhat related so maybe we can start by doing
> > that?
> >
> 
> Possibly... that encodes to some extent the referencing semantics, so it
> would be useful to have it all in one place. Though it sounds like we still
> need a "before CreateImage" (e.g. dup the fd) and a "after CreateImage"
> (e.g. insert and stash sync points) parts.
> OTOH, you need the GpuChannelHost to be able to do the
> ShareGpuMemoryBufferToGpuProcess (because you need the actual channel to be
> able to dup on Windows). You'll also need the channel to make a sync point.
> So you'll need to tie the GpuMemoryBufferImpl to the channel (or at least
> these 2 operations, may be ok?).

Yes, there's clearly some overlap here.

As we already have ShareGpuMemoryBufferToGpuProcess with type specific code, I'm
fine adding something similar to GpuChannelHost for the sync point case. Like
just stash a sync point in the GpuChannelHost when issuing a CreateImage for a
specific type. And the specific GpuMemoryBufferImpl could then just pull the
latest sync point of the GpuChannelHost at the time of destruction. That would
fix destruction in the short term and we can follow up by more appropriately
integrating this with the GpuMemoryBufferImpl code later.

> 
> 
> > https://codereview.chromium.org/654223006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[alexst (slow to review), 2014-10-30 22:37:24]

This stashes the sync point on the GpuMemoryBufferImpl.

I tried stashing the last sync point added after CreateImage IPC on the
GpuChannelHost, but it turned out to be a plumbing nightmare. Keeping the sync
point along the the buffer looks to be the cleanest solution to me.

[piman, 2014-10-31 01:24:19]

https://codereview.chromium.org/654223006/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_mac.cc (right):

https://codereview.chromium.org/654223006/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_mac.cc:84: return false;
true for IO_SURFACE_BUFFER?

Do we really need a per-platform implementation of this? What really matter is
what we do relative to the handle type (whether it's an OS handle that we dup,
or it's a reference to a key in the GPU process), and that doesn't vary per
platform, so a single implementation with a single switch statement seems the
most reasonable.

IO_SURFACE_BUFFER, X11_PIXMAP_BUFFER, OZONE_NATIVE_BUFFER,
SURFACE_TEXTURE_BUFFER need a sync point.
SHARED_MEMORY_BUFFER doesn't.

[reveman, 2014-10-31 11:39:36]

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

https://codereview.chromium.org/654223006/diff/100001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:321:
channel_->ShareGpuMemoryBufferToGpuProcess(
Maybe you can pass a route_id_ or an InserSyncPointCallback to this function so
it can easily use
GpuMemoryBufferManager::SetGpuMemoryBufferDestructionSyncPointCallback as I
suggested in my other comment.

https://codereview.chromium.org/654223006/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/654223006/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_memory_buffer_manager.h:32: virtual bool
NeedsDestructionSyncPoint(gfx::GpuMemoryBufferType type) = 0;
Let's avoid this and just use the switch statement in
GpuChannelHost::ShareGpuMemoryBufferToGpuProcess instead.

https://codereview.chromium.org/654223006/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_memory_buffer_manager.h:35: virtual void
RecordDestructionSyncPoint(ClientBuffer buffer,
We shouldn't be using ClientBuffer except for C API.

https://codereview.chromium.org/654223006/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_memory_buffer_manager.h:36: uint32 sync_point) =
0;
Can we make this:

virtual void SetGpuMemoryBufferDestructionSyncPointCallback(
    gfx::GpuMemoryBufferType type,
    const gfx::GpuMemoryBufferId& id,
    const InsertSyncPointCallback& insert_sync_point_callback) = 0;

and keep these callbacks in a map in the manager instead of adding
GpuMemoryBufferImpl API. The map might seem unnecessary but we'll need it to
handle process removal anyhow (in follow up patch).

The InsertSyncPointCallback can either return a sync point directly or take a
SyncPointInserted callback depending on if you need to make a thread hop or not.
I don't think you have to make a thread hop if you just make sure to track the
route_id as part of the InsertSyncPointCallback.

The manager's buffer deletion handler would use this callback if set when
deleting a buffer.

This would allow us to handle the sync point decision as part of the existing
ShareGpuMemoryBufferToGpuProcess logic and the need for a destruction
insertsyncpoint callback can simply be decided based on the existing switch
statement in that function. It also moves the sync point creation to destruction
time, which I think is a bit better as it's not going to affect the "initial
frame up on screen" time.

[alexst (slow to review), 2014-10-31 18:03:59]

> Can we make this:
> 
> virtual void SetGpuMemoryBufferDestructionSyncPointCallback(
>     gfx::GpuMemoryBufferType type,
>     const gfx::GpuMemoryBufferId& id,
>     const InsertSyncPointCallback& insert_sync_point_callback) = 0;
> 
> and keep these callbacks in a map in the manager instead of adding
> GpuMemoryBufferImpl API. The map might seem unnecessary but we'll need it to
> handle process removal anyhow (in follow up patch).

Can you elaborate a bit on why the map would be needed for process removal?
 
> The InsertSyncPointCallback can either return a sync point directly or take a
> SyncPointInserted callback depending on if you need to make a thread hop or
not.
> I don't think you have to make a thread hop if you just make sure to track the
> route_id as part of the InsertSyncPointCallback.

Why worry about keeping track of route_id', thread hops and lifecycles of
objects inside these callbacks, when we are already in CommandBufferProxyImpl
that can insert a sync point and know the buffer that needs it. Is it to avoid
sync point at creation? Is there really a performance impact to it?

Stashing this callback for future use on the manager would also introduce
layering issues and further API changed on the browser side.
GpuMemoryBufferImplOzoneNativeBuffer which is in content/common/ would need to
have access to the BrowserGpuMemoryBufferManager for browser allocated objects.
Solving that would probably require a new API in GpuMemoryBufferFactoryHost
because only it can get the buffer manager without layering violations to insert
the sync point.

Pending issues with process removal, I still think the cleanest thing is to keep
the sync point with the GpuMemoryBufferImpl object and insert it immediately on
the command buffer when needed.

[reveman, 2014-10-31 19:28:41]

On 2014/10/31 18:03:59, alexst wrote:
> > Can we make this:
> > 
> > virtual void SetGpuMemoryBufferDestructionSyncPointCallback(
> >     gfx::GpuMemoryBufferType type,
> >     const gfx::GpuMemoryBufferId& id,
> >     const InsertSyncPointCallback& insert_sync_point_callback) = 0;
> > 
> > and keep these callbacks in a map in the manager instead of adding
> > GpuMemoryBufferImpl API. The map might seem unnecessary but we'll need it to
> > handle process removal anyhow (in follow up patch).
> 
> Can you elaborate a bit on why the map would be needed for process removal?
>  
> > The InsertSyncPointCallback can either return a sync point directly or take
a
> > SyncPointInserted callback depending on if you need to make a thread hop or
> not.
> > I don't think you have to make a thread hop if you just make sure to track
the
> > route_id as part of the InsertSyncPointCallback.
> 
> Why worry about keeping track of route_id', thread hops and lifecycles of
> objects inside these callbacks, when we are already in CommandBufferProxyImpl
> that can insert a sync point and know the buffer that needs it. Is it to avoid
> sync point at creation? Is there really a performance impact to it?

It's a round-trip to the GPU process for every tile we create. I'd be surprised
if that doesn't have an impact on the time it takes to produce a first frame.
However, you can insert the sync point right away if you prefer that though.
What I'm proposing doesn't prevent that.

The main reason is that if we do it this way it becomes part of the existing
ShareToGpuProcess mechanism instead of us having to introduce something new. And
I like that a lot. It also means that we can take advantage of the existing
switch statement (which we should try to remove later) to keep things simple.

> 
> Stashing this callback for future use on the manager would also introduce
> layering issues and further API changed on the browser side.
> GpuMemoryBufferImplOzoneNativeBuffer which is in content/common/ would need to
> have access to the BrowserGpuMemoryBufferManager for browser allocated
objects.

The idea is that it would all be contained within the manager. You'd have to
adjust the DestructionCallback and fix so you can pass one to
GpuMemoryBufferImpl::Create. The DestructionCallback is basically a delegate. If
we have it take a DestroyCallback as parameter, that gives us a clean way to
handle the case where we have to tell GpuMemoryBufferFactoryHost to destroy the
buffer.

> Solving that would probably require a new API in GpuMemoryBufferFactoryHost
> because only it can get the buffer manager without layering violations to
insert
> the sync point.
> 
> Pending issues with process removal, I still think the cleanest thing is to
keep
> the sync point with the GpuMemoryBufferImpl object and insert it immediately
on
> the command buffer when needed.

Ok, let's insert the sync point immediately for now but I would prefer to keep
track of this in the manager instead of stashing it in the buffer if possible.
If you feel like it's necessary to store the sync point in the buffer, then do
that for now but please put a TODO(reveman) next to it so I remember to follow
up with what I have in mind.

[alexst (slow to review), 2014-10-31 21:24:56]

> The main reason is that if we do it this way it becomes part of the existing
> ShareToGpuProcess mechanism instead of us having to introduce something new.
And
> I like that a lot. It also means that we can take advantage of the existing
> switch statement (which we should try to remove later) to keep things simple.

To make sure I understand your proposal, is the flow something like this:

Since ShareGpuMemoryBufferToGpuProcess happens before
GpuCommandBufferMsg_CreateImage, we pass a callback to insert a sync point into
ShareGpuMemoryBufferToGpuProcess, which will stash it on manager via
SetGpuMemoryBufferDestructionSyncPointCallback(type, id, callback) only for the
types that requires it, and a null callback for those that don't.

We would then call something on the manager after
GpuCommandBufferMsg_CreateImage to execute this callback and store a sync point
for this buffer type and id in a map on the manager. (I think we have to call it
right after CreateImage, otherwise we have to deal with lifecycle issues of
objects in the callback if we call it later)

We also modify GpuMemoryBufferImpl::Create(..., DestructionCallback). This
callback can be called during destruction with type and id to look up the sync
point information from the manager during destruction in order to not store it
on the GpuMemoryBufferImpl object. Or were you thinking that this destruction
callback be used in some other way?

[reveman, 2014-10-31 22:22:23]

On 2014/10/31 21:24:56, alexst wrote:
> > The main reason is that if we do it this way it becomes part of the existing
> > ShareToGpuProcess mechanism instead of us having to introduce something new.
> And
> > I like that a lot. It also means that we can take advantage of the existing
> > switch statement (which we should try to remove later) to keep things
simple.
> 
> To make sure I understand your proposal, is the flow something like this:
> 
> Since ShareGpuMemoryBufferToGpuProcess happens before
> GpuCommandBufferMsg_CreateImage, we pass a callback to insert a sync point
into
> ShareGpuMemoryBufferToGpuProcess, which will stash it on manager via
> SetGpuMemoryBufferDestructionSyncPointCallback(type, id, callback) only for
the
> types that requires it, and a null callback for those that don't.

Yes, but we could just avoid calling
SetGpuMemoryBufferDestructionSyncPointCallback for types that don't need it. We
might want to make this a bit more generic in the future (have the name be
SetGpuMemoryBufferDestructionDelegate) but no need to worry about this now.

> 
> We would then call something on the manager after
> GpuCommandBufferMsg_CreateImage to execute this callback and store a sync
point
> for this buffer type and id in a map on the manager. (I think we have to call
it
> right after CreateImage, otherwise we have to deal with lifecycle issues of
> objects in the callback if we call it later)

The idea would be to keep the callback around until the buffer is actually
destroyed. That callback can eventually be doing something smarter but for now
it would simply return the sync point created after
GpuCommandBufferMsg_CreateImage. If you pass a base:Value ref to the callback
it's easy to set that to the correct sync point after CreateImage has been
issued. Similar to what I think you did in some patch with an SyncPointTracker
object..

> 
> We also modify GpuMemoryBufferImpl::Create(..., DestructionCallback). This
> callback can be called during destruction with type and id to look up the sync
> point information from the manager during destruction in order to not store it
> on the GpuMemoryBufferImpl object. Or were you thinking that this destruction
> callback be used in some other way?

Yes, that's the idea. ~GpuMemoryBufferImplOzoneNativeBuffer would run the
DestructionCallback and pass it another callback as parameter, a
DestroyCallback. This destroy callback would be called by the destruction
callback using the correct sync point. The destroy callback will call the
Factory method issue the destroy on the GPU process side.

[alexst (slow to review), 2014-10-31 22:39:21]

On 2014/10/31 22:22:23, reveman wrote:
> On 2014/10/31 21:24:56, alexst wrote:
> > > The main reason is that if we do it this way it becomes part of the
existing
> > > ShareToGpuProcess mechanism instead of us having to introduce something
new.
> > And
> > > I like that a lot. It also means that we can take advantage of the
existing
> > > switch statement (which we should try to remove later) to keep things
> simple.
> > 
> > To make sure I understand your proposal, is the flow something like this:
> > 
> > Since ShareGpuMemoryBufferToGpuProcess happens before
> > GpuCommandBufferMsg_CreateImage, we pass a callback to insert a sync point
> into
> > ShareGpuMemoryBufferToGpuProcess, which will stash it on manager via
> > SetGpuMemoryBufferDestructionSyncPointCallback(type, id, callback) only for
> the
> > types that requires it, and a null callback for those that don't.
> 
> Yes, but we could just avoid calling
> SetGpuMemoryBufferDestructionSyncPointCallback for types that don't need it.
We
> might want to make this a bit more generic in the future (have the name be
> SetGpuMemoryBufferDestructionDelegate) but no need to worry about this now.
> 
> > 
> > We would then call something on the manager after
> > GpuCommandBufferMsg_CreateImage to execute this callback and store a sync
> point
> > for this buffer type and id in a map on the manager. (I think we have to
call
> it
> > right after CreateImage, otherwise we have to deal with lifecycle issues of
> > objects in the callback if we call it later)
> 
> The idea would be to keep the callback around until the buffer is actually
> destroyed. That callback can eventually be doing something smarter but for now
> it would simply return the sync point created after
> GpuCommandBufferMsg_CreateImage. If you pass a base:Value ref to the callback
> it's easy to set that to the correct sync point after CreateImage has been
> issued. Similar to what I think you did in some patch with an SyncPointTracker
> object..
> 
> > 
> > We also modify GpuMemoryBufferImpl::Create(..., DestructionCallback). This
> > callback can be called during destruction with type and id to look up the
sync
> > point information from the manager during destruction in order to not store
it
> > on the GpuMemoryBufferImpl object. Or were you thinking that this
destruction
> > callback be used in some other way?
> 
> Yes, that's the idea. ~GpuMemoryBufferImplOzoneNativeBuffer would run the
> DestructionCallback and pass it another callback as parameter, a
> DestroyCallback. This destroy callback would be called by the destruction
> callback using the correct sync point. The destroy callback will call the
> Factory method issue the destroy on the GPU process side.

Okay, if Antoine doesn't have objections to this plan, I will implement it.

[piman, 2014-10-31 23:56:14]

On Fri, Oct 31, 2014 at 3:39 PM, <alexst@chromium.org> wrote:

> On 2014/10/31 22:22:23, reveman wrote:
>
>> On 2014/10/31 21:24:56, alexst wrote:
>> > > The main reason is that if we do it this way it becomes part of the
>>
> existing
>
>> > > ShareToGpuProcess mechanism instead of us having to introduce
>> something
>>
> new.
>
>> > And
>> > > I like that a lot. It also means that we can take advantage of the
>>
> existing
>
>> > > switch statement (which we should try to remove later) to keep things
>> simple.
>> >
>> > To make sure I understand your proposal, is the flow something like
>> this:
>> >
>> > Since ShareGpuMemoryBufferToGpuProcess happens before
>> > GpuCommandBufferMsg_CreateImage, we pass a callback to insert a sync
>> point
>> into
>> > ShareGpuMemoryBufferToGpuProcess, which will stash it on manager via
>> > SetGpuMemoryBufferDestructionSyncPointCallback(type, id, callback)
>> only for
>> the
>> > types that requires it, and a null callback for those that don't.
>>
>
>  Yes, but we could just avoid calling
>> SetGpuMemoryBufferDestructionSyncPointCallback for types that don't need
>> it.
>>
> We
>
>> might want to make this a bit more generic in the future (have the name be
>> SetGpuMemoryBufferDestructionDelegate) but no need to worry about this
>> now.
>>
>
>  >
>> > We would then call something on the manager after
>> > GpuCommandBufferMsg_CreateImage to execute this callback and store a
>> sync
>> point
>> > for this buffer type and id in a map on the manager. (I think we have to
>>
> call
>
>> it
>> > right after CreateImage, otherwise we have to deal with lifecycle
>> issues of
>> > objects in the callback if we call it later)
>>
>
>  The idea would be to keep the callback around until the buffer is actually
>> destroyed. That callback can eventually be doing something smarter but
>> for now
>> it would simply return the sync point created after
>> GpuCommandBufferMsg_CreateImage. If you pass a base:Value ref to the
>> callback
>> it's easy to set that to the correct sync point after CreateImage has been
>> issued. Similar to what I think you did in some patch with an
>> SyncPointTracker
>> object..
>>
>
>  >
>> > We also modify GpuMemoryBufferImpl::Create(..., DestructionCallback).
>> This
>> > callback can be called during destruction with type and id to look up
>> the
>>
> sync
>
>> > point information from the manager during destruction in order to not
>> store
>>
> it
>
>> > on the GpuMemoryBufferImpl object. Or were you thinking that this
>>
> destruction
>
>> > callback be used in some other way?
>>
>
>  Yes, that's the idea. ~GpuMemoryBufferImplOzoneNativeBuffer would run the
>> DestructionCallback and pass it another callback as parameter, a
>> DestroyCallback. This destroy callback would be called by the destruction
>> callback using the correct sync point. The destroy callback will call the
>> Factory method issue the destroy on the GPU process side.
>>
>
> Okay, if Antoine doesn't have objections to this plan, I will implement it.
>

TBH, maps and base::Value and callbacks calling callbacks feel like an
overly complex design, and I think it's because of premature
generalization. Just naming all these concepts become hard. And it makes
the code impossible to comprehend for anyone who isn't intimately familiar
with this discussion.

We only need 2 ways of synchronizing right now, either with nothing (shm)
or with a sync point (everything else).
Why can't we just add a sync point to GpuMemoryBufferImpl (for all the
non-shm cases), and be done with it?

We can generalize later if/when we actually need it.


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

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

[reveman, 2014-11-01 22:43:49]

On 2014/10/31 23:56:14, piman (Very slow to review) wrote:
> On Fri, Oct 31, 2014 at 3:39 PM, <mailto:alexst@chromium.org> wrote:
> 
> > On 2014/10/31 22:22:23, reveman wrote:
> >
> >> On 2014/10/31 21:24:56, alexst wrote:
> >> > > The main reason is that if we do it this way it becomes part of the
> >>
> > existing
> >
> >> > > ShareToGpuProcess mechanism instead of us having to introduce
> >> something
> >>
> > new.
> >
> >> > And
> >> > > I like that a lot. It also means that we can take advantage of the
> >>
> > existing
> >
> >> > > switch statement (which we should try to remove later) to keep things
> >> simple.
> >> >
> >> > To make sure I understand your proposal, is the flow something like
> >> this:
> >> >
> >> > Since ShareGpuMemoryBufferToGpuProcess happens before
> >> > GpuCommandBufferMsg_CreateImage, we pass a callback to insert a sync
> >> point
> >> into
> >> > ShareGpuMemoryBufferToGpuProcess, which will stash it on manager via
> >> > SetGpuMemoryBufferDestructionSyncPointCallback(type, id, callback)
> >> only for
> >> the
> >> > types that requires it, and a null callback for those that don't.
> >>
> >
> >  Yes, but we could just avoid calling
> >> SetGpuMemoryBufferDestructionSyncPointCallback for types that don't need
> >> it.
> >>
> > We
> >
> >> might want to make this a bit more generic in the future (have the name be
> >> SetGpuMemoryBufferDestructionDelegate) but no need to worry about this
> >> now.
> >>
> >
> >  >
> >> > We would then call something on the manager after
> >> > GpuCommandBufferMsg_CreateImage to execute this callback and store a
> >> sync
> >> point
> >> > for this buffer type and id in a map on the manager. (I think we have to
> >>
> > call
> >
> >> it
> >> > right after CreateImage, otherwise we have to deal with lifecycle
> >> issues of
> >> > objects in the callback if we call it later)
> >>
> >
> >  The idea would be to keep the callback around until the buffer is actually
> >> destroyed. That callback can eventually be doing something smarter but
> >> for now
> >> it would simply return the sync point created after
> >> GpuCommandBufferMsg_CreateImage. If you pass a base:Value ref to the
> >> callback
> >> it's easy to set that to the correct sync point after CreateImage has been
> >> issued. Similar to what I think you did in some patch with an
> >> SyncPointTracker
> >> object..
> >>
> >
> >  >
> >> > We also modify GpuMemoryBufferImpl::Create(..., DestructionCallback).
> >> This
> >> > callback can be called during destruction with type and id to look up
> >> the
> >>
> > sync
> >
> >> > point information from the manager during destruction in order to not
> >> store
> >>
> > it
> >
> >> > on the GpuMemoryBufferImpl object. Or were you thinking that this
> >>
> > destruction
> >
> >> > callback be used in some other way?
> >>
> >
> >  Yes, that's the idea. ~GpuMemoryBufferImplOzoneNativeBuffer would run the
> >> DestructionCallback and pass it another callback as parameter, a
> >> DestroyCallback. This destroy callback would be called by the destruction
> >> callback using the correct sync point. The destroy callback will call the
> >> Factory method issue the destroy on the GPU process side.
> >>
> >
> > Okay, if Antoine doesn't have objections to this plan, I will implement it.
> >
> 
> TBH, maps and base::Value and callbacks calling callbacks feel like an
> overly complex design, and I think it's because of premature
> generalization. Just naming all these concepts become hard. And it makes
> the code impossible to comprehend for anyone who isn't intimately familiar
> with this discussion.
> 
> We only need 2 ways of synchronizing right now, either with nothing (shm)
> or with a sync point (everything else).
> Why can't we just add a sync point to GpuMemoryBufferImpl (for all the
> non-shm cases), and be done with it?
> 
> We can generalize later if/when we actually need it.
> 

Ok, makes sense.

Alex, if you could just handle this sync point decision in a way that makes use
of the existing ShareGpuMemoryBufferToGpuProcess function (maybe have that
function return true if a sync point is required) then this about ready to land,
I think.

[alexst (slow to review), 2014-11-03 16:34:37]

PTAL

Moved the decision into ShareGpuMemoryBufferToGpuProcess.

https://codereview.chromium.org/654223006/diff/180001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/654223006/diff/180001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_memory_buffer_manager.h:31: virtual void
RecordDestructionSyncPoint(ClientBuffer buffer,
Dave, you had a concern over usage of ClientBuffer type here in an earlier rev,
would you like it handled differently, or is this okay?

[piman, 2014-11-03 19:36:35]

Cool. Couple of things.

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:70: sync_point);
What prevents an arbitrary renderer from destroying a memory buffer belonging to
another process? Should we pass through the child id (as opposed to the
child_process which I don't think is useful), and check against id.secondary_id?
(On a per-type basis, I suppose?)

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:115: }
I think both GpuMemoryBufferImplSurfaceTexture and GpuMemoryBufferImplIOSurface
need the same logic.

[alexst (slow to review), 2014-11-03 20:27:10]

Patchset #10 (id:200001) has been deleted

[alexst (slow to review), 2014-11-03 20:29:17]

Thanks for the comments, new patch.

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:70: sync_point);
On 2014/11/03 19:36:34, piman (Very slow to review) wrote:
> What prevents an arbitrary renderer from destroying a memory buffer belonging
to
> another process? Should we pass through the child id (as opposed to the
> child_process which I don't think is useful), and check against
id.secondary_id?
> (On a per-type basis, I suppose?)

Good point, AllocateForChildProcess takes both ProcessHandle and
child_client_id, so I added child_client_id here for symmetry and introduced the
check.

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/654223006/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:115: }
On 2014/11/03 19:36:35, piman (Very slow to review) wrote:
> I think both GpuMemoryBufferImplSurfaceTexture and
GpuMemoryBufferImplIOSurface
> need the same logic.

Indeed, added missing code.

[piman, 2014-11-03 20:39:47]

LGTM, thanks!

[alexst (slow to review), 2014-11-03 22:07:09]

alexst@chromium.org changed reviewers:
+ jschuh@chromium.org, kenrb@chromium.org

[alexst (slow to review), 2014-11-03 22:07:53]

+jschuh or kenrb for owner of

content/common/child_process_messages.h

[alexst (slow to review), 2014-11-03 22:07:54]

+jschuh or kenrb for owner of

content/common/child_process_messages.h

[kenrb, 2014-11-03 22:08:59]

ipc lgtm

[reveman, 2014-11-03 22:26:12]

lgtm with nits

https://codereview.chromium.org/654223006/diff/260001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/654223006/diff/260001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:119: child_client_id,
sync_point);
nit: is this formatted correctly?

https://codereview.chromium.org/654223006/diff/260001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc (right):

https://codereview.chromium.org/654223006/diff/260001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc:31: callback));
nit: cl format?

https://codereview.chromium.org/654223006/diff/260001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc:67:
base::Unretained(this), handle, sync_point));
nit: cl format?

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

https://codereview.chromium.org/654223006/diff/260001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:311:
gpu::GpuMemoryBufferManager* memory_buffer_manager =
nit: do you need this temporary variable? if you do, rename it to
"gpu_memory_buffer_manager"

https://codereview.chromium.org/654223006/diff/260001/content/common/gpu/clie...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/654223006/diff/260001/content/common/gpu/clie...
content/common/gpu/client/gpu_channel_host.cc:286: DCHECK(requires_sync_point);
nit: I think this DCHECK is overkill

https://codereview.chromium.org/654223006/diff/260001/content/common/gpu/clie...
content/common/gpu/client/gpu_channel_host.cc:317: *requires_sync_point = false;
nit: I don't think you should set requires_sync_point in the error case

https://codereview.chromium.org/654223006/diff/260001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/654223006/diff/260001/gpu/command_buffer/clie...
gpu/command_buffer/client/gpu_memory_buffer_manager.h:32: uint32 sync_point) =
0;
nit: s/ClientBuffer/gfx::GpuMemoryBuffer*/ and rename to
SetDestructionSyncPoint. "Record" sounds like calling this multiple times
doesn't necessarily replace the previous sync point and it's called set at a
lower level so it's better to be consistent

[alexst (slow to review), 2014-11-03 22:47:42]

I'll address the comments shortly, but as far as formatting goes, I ran it
through cl format, and again just now to be sure. It did modify > 80 line things
I had in the file, so it is working.

https://codereview.chromium.org/654223006/diff/260001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc (right):

https://codereview.chromium.org/654223006/diff/260001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc:67:
base::Unretained(this), handle, sync_point));
On 2014/11/03 22:26:12, reveman wrote:
> nit: cl format?

I was as surprised as you, this is the result of 'git cl format'

[alexst (slow to review), 2014-11-03 23:46:42]

patch

[reveman, 2014-11-04 00:16:13]

still lgtm but with a few minor requests

https://codereview.chromium.org/654223006/diff/280001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/654223006/diff/280001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:119: child_client_id,
sync_point);
please keep the style consistent with current code in this file for now and
we'll reformat the whole file in a separate patch if this is actually the new
correct format.

https://codereview.chromium.org/654223006/diff/280001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc (right):

https://codereview.chromium.org/654223006/diff/280001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc:31: callback));
please keep style consistent with current code in this file for now.

https://codereview.chromium.org/654223006/diff/280001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.cc:67:
base::Unretained(this), handle, sync_point));
please keep style consistent with current code in this file for now.

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_android.cc (right):

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:79: LOG(ERROR) <<
"Unexpected GpuMemoryBufferType";
NOTREACHED() as this shouldn't be able to happen.

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (right):

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:114: 
nit: make blank line usage consistent with Create/AllocateForChildProcess

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_mac.cc (right):

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_mac.cc:75: LOG(ERROR) <<
"Unexpected GpuMemoryBufferType";
NOTREACHED

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:79: LOG(ERROR) <<
"Unexpected GpuMemoryBufferType";
NOTREACHED();

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:112: 
nit: make blank line usage consistent with Create/AllocateForChildProcess

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.cc
(right):

https://codereview.chromium.org/654223006/diff/280001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.cc:129: 
nit: make blank line usage consistent with Create/AllocateForChildProcess

[alexst (slow to review), 2014-11-04 01:59:02]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-11-04 02:01:44]

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

[commit-bot: I haz the power, 2014-11-04 03:01:10]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2014-11-04 03:02:25]

Patchset 14 (id:??) landed as
https://crrev.com/2992e56e7d52e3158400980b312ecf0a8ddbfe1d
Cr-Commit-Position: refs/heads/master@{#302561}