Mojo C++ bindings: introduce mojo::internal::deque and use it in MultiplexRouter.

Mojo C++ bindings: introduce mojo::internal::deque and use it in MultiplexRouter.

The reason is that std::deque is memory inefficient.

BUG=719774
Review-Url: https://codereview.chromium.org/2900543002
Cr-Commit-Position: refs/heads/master@{#474527}
Committed: https://chromium.googlesource.com/chromium/src/+/7a6de50a928be5382714446119f704df13973205

[yzshen1, 2017-05-21 06:39:10]

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

[commit-bot: I haz the power, 2017-05-21 06:39:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2017-05-21 06:39:55]

yzshen@chromium.org changed reviewers:
+ rockot@chromium.org

[yzshen1, 2017-05-21 06:39:56]

Hi, Ken.

Would you please take a look?
Thanks!

[commit-bot: I haz the power, 2017-05-21 08:14:33]

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

[commit-bot: I haz the power, 2017-05-21 08:14:34]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-05-22 16:45:28]

LGTM but I wonder if there's really any need to call this "PointerDeque" or
limit it owned or unowned pointers? It seems like this easily generalizes over
any movable type T.

I think ranges of trivially copyable things like pointers will be memcpy'd if
you std::move them, and I think you should be fine with something like
buffer_[index].~T() to destroy any element type. Then there would be no need for
the traits and it could be more easily generalized if and when it's time to move
to //base?

https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/pointer_deque.h (right):

https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/pointer_deque.h:87: if (this != &other) {
I think we discourage self-assignment checks in move assignment operators:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc

DCHECK is sufficient if you like

https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/pointer_deque.h:206: size_t original_size =
buffer_.size();
nit: Good use for CheckedNumeric in base/numerics/safe_math.h. e.g:

base::CheckedNumeric<size_t> new_size = original_size;
new_size *= 2;
buffer_.resize(new_size.ValueOrDie());

[yzshen1, 2017-05-22 17:07:41]

On 2017/05/22 16:45:28, Ken Rockot(use gerrit already) wrote:
> LGTM but I wonder if there's really any need to call this "PointerDeque" or
> limit it owned or unowned pointers? It seems like this easily generalizes over
> any movable type T.

> 
> I think ranges of trivially copyable things like pointers will be memcpy'd if
> you std::move them, and I think you should be fine with something like
> buffer_[index].~T() to destroy any element type. Then there would be no need
for
> the traits and it could be more easily generalized if and when it's time to
move
> to //base?

Yeah. It shouldn't be much harder. I could do that. I will update this CL and
ping you again. :) Thanks!


> 
>
https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
> File mojo/public/cpp/bindings/lib/pointer_deque.h (right):
> 
>
https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
> mojo/public/cpp/bindings/lib/pointer_deque.h:87: if (this != &other) {
> I think we discourage self-assignment checks in move assignment operators:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc
> 
> DCHECK is sufficient if you like
> 
>
https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
> mojo/public/cpp/bindings/lib/pointer_deque.h:206: size_t original_size =
> buffer_.size();
> nit: Good use for CheckedNumeric in base/numerics/safe_math.h. e.g:
> 
> base::CheckedNumeric<size_t> new_size = original_size;
> new_size *= 2;
> buffer_.resize(new_size.ValueOrDie());

[yzshen1, 2017-05-24 23:13:33]

Description was changed from

==========
Mojo C++ bindings: introduce PointerDeque and use it in MultiplexRouter.

The reason is that std::deque is memory inefficient.

BUG=719774
==========

to

==========
Mojo C++ bindings: introduce mojo::internal::deque and use it in
MultiplexRouter.

The reason is that std::deque is memory inefficient.

BUG=719774
==========

[yzshen1, 2017-05-24 23:45:51]

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

[commit-bot: I haz the power, 2017-05-24 23:46:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2017-05-25 00:06:44]

Hi, Ken.

Would you please take a look? Thanks!

I didn't use a special storage type to avoid default-constructions. I will do
that in a follow-up CL when I have got a chance.

And I didn't use std::vector::reserve instead of resize: without a special
storage type it still doesn't avoid default constructions in some cases (for
example, imagine the buffer is fully occupied and front is at index 0, if we
push_front() in this case, we will expand the buffer and put the new element at
the very end).

https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/pointer_deque.h (right):

https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/pointer_deque.h:87: if (this != &other) {
On 2017/05/22 16:45:28, Ken Rockot(use gerrit already) wrote:
> I think we discourage self-assignment checks in move assignment operators:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc
> 
> DCHECK is sufficient if you like

Done.

(It seems the discussion also came to the conclusion that we shouldn't DCHECK.)

https://codereview.chromium.org/2900543002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/pointer_deque.h:206: size_t original_size =
buffer_.size();
On 2017/05/22 16:45:28, Ken Rockot(use gerrit already) wrote:
> nit: Good use for CheckedNumeric in base/numerics/safe_math.h. e.g:
> 
> base::CheckedNumeric<size_t> new_size = original_size;
> new_size *= 2;
> buffer_.resize(new_size.ValueOrDie());

Done.

[Ken Rockot(use gerrit already), 2017-05-25 01:57:39]

lgtm

[commit-bot: I haz the power, 2017-05-25 01:58:33]

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

[commit-bot: I haz the power, 2017-05-25 01:58:33]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-05-25 02:16:56]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2017-05-25 02:17:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 02:23:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495678616268480,
"parent_rev": "840177e705e9f12b06d2c80c6cd0d5c3d6b8b9e3", "commit_rev":
"7a6de50a928be5382714446119f704df13973205"}

[commit-bot: I haz the power, 2017-05-25 02:23:15]

Description was changed from

==========
Mojo C++ bindings: introduce mojo::internal::deque and use it in
MultiplexRouter.

The reason is that std::deque is memory inefficient.

BUG=719774
==========

to

==========
Mojo C++ bindings: introduce mojo::internal::deque and use it in
MultiplexRouter.

The reason is that std::deque is memory inefficient.

BUG=719774

Review-Url: https://codereview.chromium.org/2900543002
Cr-Commit-Position: refs/heads/master@{#474527}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a6de50a928be5382714446119f7...
==========

[commit-bot: I haz the power, 2017-05-25 02:23:17]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7a6de50a928be5382714446119f7...