CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

As soon as the GpuMemoryBuffer is destroyed, the cached GpuMemoryBuffer in the
browser process is also cleaned up, despite the fact that underlying IOSurface
is still usable. This keeps the GpuMemoryBuffer around in the child process as
long as there is an associated image.

BUG=581777
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/d3c17c8595858ca07f0c25e2b8c48ae8f281fcee
Cr-Commit-Position: refs/heads/master@{#398978}

[erikchen, 2016-06-06 17:06:59]

Description was changed from

==========
CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

As soon as the GpuMemoryBuffer is destroyed, the cached GpuMemoryBuffer in the
browser process is also cleaned up, despite the fact that underlying IOSurface
is still usable.

BUG=
==========

to

==========
CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

As soon as the GpuMemoryBuffer is destroyed, the cached GpuMemoryBuffer in the
browser process is also cleaned up, despite the fact that underlying IOSurface
is still usable.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-06 17:07:58]

Description was changed from

==========
CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

As soon as the GpuMemoryBuffer is destroyed, the cached GpuMemoryBuffer in the
browser process is also cleaned up, despite the fact that underlying IOSurface
is still usable.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

As soon as the GpuMemoryBuffer is destroyed, the cached GpuMemoryBuffer in the
browser process is also cleaned up, despite the fact that underlying IOSurface
is still usable. This keeps the GpuMemoryBuffer around in the child process as
long as there is an associated image.

BUG=581777
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-06 17:10:40]

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

[erikchen, 2016-06-06 17:10:50]

erikchen@chromium.org changed reviewers:
+ ccameron@chromium.org

[erikchen, 2016-06-06 17:10:51]

ccameron: Please review.

[commit-bot: I haz the power, 2016-06-06 17:11:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041043002/20001

[commit-bot: I haz the power, 2016-06-06 17:22:40]

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

[commit-bot: I haz the power, 2016-06-06 17:22:41]

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

[erikchen, 2016-06-06 17:33:23]

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

[erikchen, 2016-06-06 17:33:23]

piman: Please review.

[ccameron, 2016-06-06 17:59:53]

lgtm.

Long-term, we should probably find a way to do this in the GPU process at the
image-bind time (so that we can do this sort of fire-and-forget creation
everywhere). But that idea is probably best left as contemplation for now.

[piman, 2016-06-06 19:28:52]

LGTM overall, but a suggested improvement.

https://codereview.chromium.org/2041043002/diff/20001/gpu/ipc/client/command_...
File gpu/ipc/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/2041043002/diff/20001/gpu/ipc/client/command_...
gpu/ipc/client/command_buffer_proxy_impl.h:304: // |image_gmb_ids_map_| since
that container does not imply ownership.
nit: would it be possible to combine both maps, by having a move-only struct
that has both the GMB id and the unique_ptr?
struct ImageInfo {
  int32_t gpu_memory_buffer_id;
  std::unique_ptr<gfx::GpuMemoryBuffer> owned_gpu_memory_buffer;
};

Otherwise, can you mention that the index is the image id as well (not clear
here).

[erikchen, 2016-06-06 21:55:54]

https://codereview.chromium.org/2041043002/diff/20001/gpu/ipc/client/command_...
File gpu/ipc/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/2041043002/diff/20001/gpu/ipc/client/command_...
gpu/ipc/client/command_buffer_proxy_impl.h:304: // |image_gmb_ids_map_| since
that container does not imply ownership.
On 2016/06/06 19:28:52, piman OOO back 2016-6-2 wrote:
> nit: would it be possible to combine both maps, by having a move-only struct
> that has both the GMB id and the unique_ptr?
> struct ImageInfo {
>   int32_t gpu_memory_buffer_id;
>   std::unique_ptr<gfx::GpuMemoryBuffer> owned_gpu_memory_buffer;
> };
> 
> Otherwise, can you mention that the index is the image id as well (not clear
> here).

I created the ImageInfo struct as you suggested.

[erikchen, 2016-06-06 21:55:57]

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

[commit-bot: I haz the power, 2016-06-06 21:56:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041043002/40001

[piman, 2016-06-06 22:05:55]

lgtm

[commit-bot: I haz the power, 2016-06-06 22:10:32]

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

[commit-bot: I haz the power, 2016-06-06 22:10:33]

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

[erikchen, 2016-06-06 22:30:44]

On 2016/06/06 22:10:33, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Hm. DISALLOW_COPY_AND_ASSIGN makes the Linux compiles unhappy. It looks like the
implementation uses construct, then copy for operator[]. Unfortunately, emplace
is not available yet. I've removed DISALLOW_COPY_AND_ASSIGN.
"""
std::map::emplace() is not yet available on all libstdc++ versions we support. 
https://chromium-cpp.appspot.com/
"""

"""
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_pair.h:104:21:
error: call to deleted constructor of 'gpu::CommandBufferProxyImpl::ImageInfo'
      : first(__a), second(__b) { }
                    ^      ~~~
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_map.h:453:29:
note: in instantiation of member function 'std::pair<const int,
gpu::CommandBufferProxyImpl::ImageInfo>::pair' requested here
          __i = insert(__i, value_type(__k, mapped_type()));
                            ^
../../gpu/ipc/client/command_buffer_proxy_impl.cc:463:17: note: in instantiation
of member function 'std::__cxx1998::map<int,
gpu::CommandBufferProxyImpl::ImageInfo, std::less<int>,
std::allocator<std::pair<const int, gpu::CommandBufferProxyImpl::ImageInfo> >
>::operator[]' requested here
  image_gmb_map_[new_id].gpu_memory_buffer_id = gpu_memory_buffer->GetId().id;
                ^
../../gpu/ipc/client/command_buffer_proxy_impl.h:307:30: note: 'ImageInfo' has
been explicitly marked deleted here
    DISALLOW_COPY_AND_ASSIGN(ImageInfo);
"""

[erikchen, 2016-06-06 22:30:52]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-06-06 22:30:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2041043002/#ps60001
(title: "Remove DISALLOW_COPY_AND_ASSIGN.")

[commit-bot: I haz the power, 2016-06-06 22:31:56]

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

[piman, 2016-06-06 22:37:51]

On Mon, Jun 6, 2016 at 3:30 PM, <erikchen@chromium.org> wrote:

> On 2016/06/06 22:10:33, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> > linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
>
> Hm. DISALLOW_COPY_AND_ASSIGN makes the Linux compiles unhappy. It looks
> like the
> implementation uses construct, then copy for operator[]. Unfortunately,
> emplace
> is not available yet. I've removed DISALLOW_COPY_AND_ASSIGN.
>

I was surprised it worked at all actually. I think DISALLOW_COPY_AND_ASSIGN
causes the compiler to omit the move constructor which would be needed. I
think it will be disabled anyway, since ImageInfo is not copyable.


> """
> std::map::emplace() is not yet available on all libstdc++ versions we
> support.
> https://chromium-cpp.appspot.com/
> """
>
> """
>
>
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_pair.h:104:21:
> error: call to deleted constructor of
> 'gpu::CommandBufferProxyImpl::ImageInfo'
> : first(__a), second(__b) { }
> ^ ~~~
>
>
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_map.h:453:29:
> note: in instantiation of member function 'std::pair<const int,
> gpu::CommandBufferProxyImpl::ImageInfo>::pair' requested here
> __i = insert(__i, value_type(__k, mapped_type()));
> ^
> ../../gpu/ipc/client/command_buffer_proxy_impl.cc:463:17: note: in
> instantiation
> of member function 'std::__cxx1998::map<int,
> gpu::CommandBufferProxyImpl::ImageInfo, std::less<int>,
> std::allocator<std::pair<const int,
> gpu::CommandBufferProxyImpl::ImageInfo> >
> >::operator[]' requested here
> image_gmb_map_[new_id].gpu_memory_buffer_id =
> gpu_memory_buffer->GetId().id;
> ^
> ../../gpu/ipc/client/command_buffer_proxy_impl.h:307:30: note: 'ImageInfo'
> has
> been explicitly marked deleted here
> DISALLOW_COPY_AND_ASSIGN(ImageInfo);
> """
>
> https://codereview.chromium.org/2041043002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-06-06 22:47:41]

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

[commit-bot: I haz the power, 2016-06-06 22:47:42]

Try jobs failed on following builders:
  linux_optional_gpu_tests_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[erikchen, 2016-06-07 02:00:50]

On 2016/06/06 22:37:51, piman OOO back 2016-6-2 wrote:
> On Mon, Jun 6, 2016 at 3:30 PM, <mailto:erikchen@chromium.org> wrote:
> 
> > On 2016/06/06 22:10:33, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > > linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > )
> >
> > Hm. DISALLOW_COPY_AND_ASSIGN makes the Linux compiles unhappy. It looks
> > like the
> > implementation uses construct, then copy for operator[]. Unfortunately,
> > emplace
> > is not available yet. I've removed DISALLOW_COPY_AND_ASSIGN.
> >
> 
> I was surprised it worked at all actually. I think DISALLOW_COPY_AND_ASSIGN
> causes the compiler to omit the move constructor which would be needed. I
> think it will be disabled anyway, since ImageInfo is not copyable.

std::map on Linux requires a copy constructor.
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

I could implement one with using std::move, which would be semantically wrong
but fix the compile error. I think it's better to use two maps, as in patch set
2. wdyt?

[piman, 2016-06-07 02:37:15]

On Mon, Jun 6, 2016 at 7:00 PM, <erikchen@chromium.org> wrote:

> On 2016/06/06 22:37:51, piman OOO back 2016-6-2 wrote:
> > On Mon, Jun 6, 2016 at 3:30 PM, <mailto:erikchen@chromium.org> wrote:
> >
> > > On 2016/06/06 22:10:33, commit-bot: I haz the power wrote:
> > > > Dry run: Try jobs failed on following builders:
> > > > linux_chromium_compile_dbg_ng on tryserver.chromium.linux
> (JOB_FAILED,
> > > >
> > >
> > >
> >
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > > )
> > >
> > > Hm. DISALLOW_COPY_AND_ASSIGN makes the Linux compiles unhappy. It looks
> > > like the
> > > implementation uses construct, then copy for operator[]. Unfortunately,
> > > emplace
> > > is not available yet. I've removed DISALLOW_COPY_AND_ASSIGN.
> > >
> >
> > I was surprised it worked at all actually. I think
> DISALLOW_COPY_AND_ASSIGN
> > causes the compiler to omit the move constructor which would be needed. I
> > think it will be disabled anyway, since ImageInfo is not copyable.
>
> std::map on Linux requires a copy constructor.
>
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
>

Actually, it just needs the struct to be movable - i.e. with a move
constructor and a move assignment operator.

Something like:

struct ImageInfo {
  ImageInfo() {}
  ~ImageInfo() {}
  ImageInfo(ImageInfo&& other) { *this = std::move(other); }
  ImageInfo& operator=(ImageInfo&& other) {
    gpu_memory_buffer_id = other.gpu_memory_buffer_id;
    owned_gpu_memory_buffer = std::move(other.owned_gpu_memory_buffer);
    return *this;
  }
  int32_t gpu_memory_buffer_id = -1;
  std::unique_ptr<gfx::GpuMemoryBuffer> owned_gpu_memory_buffer;
};

Antoine


>
> I could implement one with using std::move, which would be semantically
> wrong
> but fix the compile error. I think it's better to use two maps, as in
> patch set
> 2. wdyt?
>
> https://codereview.chromium.org/2041043002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-06-07 02:45:55]

On Mon, Jun 6, 2016 at 7:36 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Jun 6, 2016 at 7:00 PM, <erikchen@chromium.org> wrote:
>
>> On 2016/06/06 22:37:51, piman OOO back 2016-6-2 wrote:
>> > On Mon, Jun 6, 2016 at 3:30 PM, <mailto:erikchen@chromium.org> wrote:
>> >
>> > > On 2016/06/06 22:10:33, commit-bot: I haz the power wrote:
>> > > > Dry run: Try jobs failed on following builders:
>> > > > linux_chromium_compile_dbg_ng on tryserver.chromium.linux
>> (JOB_FAILED,
>> > > >
>> > >
>> > >
>> >
>>
>>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
>> > > )
>> > >
>> > > Hm. DISALLOW_COPY_AND_ASSIGN makes the Linux compiles unhappy. It
>> looks
>> > > like the
>> > > implementation uses construct, then copy for operator[].
>> Unfortunately,
>> > > emplace
>> > > is not available yet. I've removed DISALLOW_COPY_AND_ASSIGN.
>> > >
>> >
>> > I was surprised it worked at all actually. I think
>> DISALLOW_COPY_AND_ASSIGN
>> > causes the compiler to omit the move constructor which would be needed.
>> I
>> > think it will be disabled anyway, since ImageInfo is not copyable.
>>
>> std::map on Linux requires a copy constructor.
>>
>>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
>>
>
> Actually, it just needs the struct to be movable - i.e. with a move
> constructor and a move assignment operator.
>
> Something like:
>
> struct ImageInfo {
>   ImageInfo() {}
>   ~ImageInfo() {}
>   ImageInfo(ImageInfo&& other) { *this = std::move(other); }
>   ImageInfo& operator=(ImageInfo&& other) {
>     gpu_memory_buffer_id = other.gpu_memory_buffer_id;
>     owned_gpu_memory_buffer = std::move(other.owned_gpu_memory_buffer);
>     return *this;
>   }
>   int32_t gpu_memory_buffer_id = -1;
>   std::unique_ptr<gfx::GpuMemoryBuffer> owned_gpu_memory_buffer;
> };
>

Actually even better, you can use the default move constructor/assignment
operators:

struct ImageInfo {
  ImageInfo() {}
  ~ImageInfo() {}
  ImageInfo(ImageInfo&& other) = default;
  ImageInfo& operator=(ImageInfo&& other) = default;
  int32_t gpu_memory_buffer_id = -1;
  std::unique_ptr<gfx::GpuMemoryBuffer> owned_gpu_memory_buffer;
};


> Antoine
>

(BTW I'll be ooo for the next couple of days, you have a lgtm on file, do
what you think is best. I think the movable type is nicer because you don't
have to lookup through the map twice for every buffer, and because of lower
overhead. The pattern is also getting more and more common in Chrome code.
But if it doesn't work for some reason go ahead with both maps).

Antoine


>
>>
>> I could implement one with using std::move, which would be semantically
>> wrong
>> but fix the compile error. I think it's better to use two maps, as in
>> patch set
>> 2. wdyt?
>>
>> https://codereview.chromium.org/2041043002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-06-07 02:46:18]

lgtm

[erikchen, 2016-06-07 16:16:16]

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

[commit-bot: I haz the power, 2016-06-07 16:32:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041043002/80001

[commit-bot: I haz the power, 2016-06-07 16:49:52]

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

[commit-bot: I haz the power, 2016-06-07 16:49:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-06-09 16:57:12]

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

[commit-bot: I haz the power, 2016-06-09 16:57:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041043002/100001

[commit-bot: I haz the power, 2016-06-09 18:30:23]

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

[commit-bot: I haz the power, 2016-06-09 18:30:25]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-06-09 19:06:03]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-06-09 19:06:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2041043002/#ps100001
(title: "Use default move constructor and move operator=.")

[commit-bot: I haz the power, 2016-06-09 19:06:28]

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

[commit-bot: I haz the power, 2016-06-09 19:11:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-09 19:15:22]

Description was changed from

==========
CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

As soon as the GpuMemoryBuffer is destroyed, the cached GpuMemoryBuffer in the
browser process is also cleaned up, despite the fact that underlying IOSurface
is still usable. This keeps the GpuMemoryBuffer around in the child process as
long as there is an associated image.

BUG=581777
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
CommandBufferProxyImpl holds on to GpuMemoryBuffers while they are in use.

As soon as the GpuMemoryBuffer is destroyed, the cached GpuMemoryBuffer in the
browser process is also cleaned up, despite the fact that underlying IOSurface
is still usable. This keeps the GpuMemoryBuffer around in the child process as
long as there is an associated image.

BUG=581777
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/d3c17c8595858ca07f0c25e2b8c48ae8f281fcee
Cr-Commit-Position: refs/heads/master@{#398978}
==========

[commit-bot: I haz the power, 2016-06-09 19:15:23]

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