Make sure we call glBufferData on the same data we store internally.

Make sure we call glBufferData on the same data we store internally.

The BufferManager now shadows the glBufferData data before we pass it
to GL so that a user cannot modify the data while we are processing it.

R=kbr@chromium.org, vmiura@chromium.org
BUG=599081
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[David Yen, 2016-03-31 22:13:18]

Description was changed from

==========
Make sure we call glBufferData on the same data we store internally.

The BufferManager now shadows the glBufferData data before we pass it
to GL so that a user cannot modify the data while we are processing it.

R=kbr@chromium.org, vmiura@chromium.org
BUG=599081
==========

to

==========
Make sure we call glBufferData on the same data we store internally.

The BufferManager now shadows the glBufferData data before we pass it
to GL so that a user cannot modify the data while we are processing it.

R=kbr@chromium.org, vmiura@chromium.org
BUG=599081
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-03-31 23:28:04]

kbr@chromium.org changed reviewers:
+ piman@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2016-03-31 23:28:05]

Thank you for taking this.

This LGTM but I am not an owner. CC'ing piman and zmo for reviews.

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:157: GLsizeiptr size, GLenum usage,
bool shadow, bool is_client_side_array) {
It might be more clear now to rename "shadow" to "use_shadow".

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:411: // Stage the shadow buffer
first if we are using a shadow buffer so that we
Is the indentation off here?

[piman, 2016-04-01 00:09:46]

Only one comment really, but the logic of the CL looks ok.

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:143: shadow_.resize(size);
Why the change to std::vector? We never need to append data to the shadow
buffer... Anyway, using a vector means that we make 2 copies here (vector.resize
copies the existing data, that we then overwrite by copying the new data or
memset).
Maybe clear() before resize to avoid the extra copy, unless the sizes match? (at
which point it's not much different than the previous logic).

[David Yen, 2016-04-01 17:00:53]

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/buffer_manager.cc (right):

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:143: shadow_.resize(size);
On 2016/04/01 00:09:46, piman wrote:
> Why the change to std::vector? We never need to append data to the shadow
> buffer... Anyway, using a vector means that we make 2 copies here
(vector.resize
> copies the existing data, that we then overwrite by copying the new data or
> memset).
> Maybe clear() before resize to avoid the extra copy, unless the sizes match?
(at
> which point it's not much different than the previous logic).

The reason I changed it to a vector was to reduce the number of memory
allocations. Essentially what I want is to have a capacity variable and size
variable. Then only reallocate memory if the size needed exceeds the capacity.
At that point it's looking more and more like std::vector. I also didn't like
how shadowed_ and shadow_ needed to be kept in lock step with one another,
although that could have been done by deleting shadow_ and checking if the
pointer is valid as well.

It's true that vector.resize() copies the existing data, but clearing and then
resizing will set each element to 0 which is effectively the same cost on new
allocations. For cases where no allocation happens this would have an extra
cost.

I agree neither case is ideal... perhaps the right thing to do is to clear and
then insert data or resize on NULL data?

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:157: GLsizeiptr size, GLenum usage,
bool shadow, bool is_client_side_array) {
On 2016/03/31 23:28:04, Ken Russell wrote:
> It might be more clear now to rename "shadow" to "use_shadow".

Done.

https://codereview.chromium.org/1845963003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/buffer_manager.cc:411: // Stage the shadow buffer
first if we are using a shadow buffer so that we
On 2016/03/31 23:28:04, Ken Russell wrote:
> Is the indentation off here?

Done. I just ran cl format.

[piman, 2016-04-01 19:40:14]

lgtm

[David Yen, 2016-04-01 19:44:19]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-04-01 19:44:19]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1845963003/#ps40001
(title: "shadow->use_shadow, Format, vector Clear+Insert/Resize")

[commit-bot: I haz the power, 2016-04-01 19:44:41]

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

[commit-bot: I haz the power, 2016-04-01 20:57:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-01 20:58:32]

Description was changed from

==========
Make sure we call glBufferData on the same data we store internally.

The BufferManager now shadows the glBufferData data before we pass it
to GL so that a user cannot modify the data while we are processing it.

R=kbr@chromium.org, vmiura@chromium.org
BUG=599081
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make sure we call glBufferData on the same data we store internally.

The BufferManager now shadows the glBufferData data before we pass it
to GL so that a user cannot modify the data while we are processing it.

R=kbr@chromium.org, vmiura@chromium.org
BUG=599081
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-04-01 20:58:33]

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