Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

Save arguments passed in through WritevData directly instead
of a list of IOBufferWithByteBuffer.

This saves unnecessary array allocations to reconstruct Java
arguments for invoking onWriteCompleted.

BUG=611056
Committed: https://crrev.com/2328a268fb12d5b99c377be5548b055c0cec9c28
Cr-Commit-Position: refs/heads/master@{#393901}

[xunjieli, 2016-05-11 19:00:56]

Description was changed from

==========
Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

BUG=611056
==========

to

==========
Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

Save arguments passed in through WritevData directly instead
of a list of IOBufferWithByteBuffer.

This saves unnecessary array allocations to reconstruct Java
arguments for invoking onWriteCompleted.

BUG=611056
==========

[xunjieli, 2016-05-11 19:00:57]

xunjieli@chromium.org changed reviewers:
+ kapishnikov@chromium.org, mef@chromium.org

[xunjieli, 2016-05-11 19:01:05]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-05-11 19:01:59]

Misha, Andrei: PTAL. Thanks!

[kapishnikov, 2016-05-11 21:15:37]

On 2016/05/11 19:01:59, xunjieli wrote:
> Misha, Andrei: PTAL. Thanks!

The change looks good to me but somebody else should take a look.

[xunjieli, 2016-05-11 21:16:17]

On 2016/05/11 21:15:37, kapishnikov wrote:
> On 2016/05/11 19:01:59, xunjieli wrote:
> > Misha, Andrei: PTAL. Thanks!
> 
> The change looks good to me but somebody else should take a look.

Sounds good! I will defer this solely to Misha then :D

[xunjieli, 2016-05-11 21:16:35]

xunjieli@chromium.org changed reviewers:
- kapishnikov@chromium.org

[mef, 2016-05-13 15:12:58]

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (left):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:278:
base::android::ToJavaIntArray(env, initial_limits);
yay!

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:65:
this->jwrite_buffer_list.Reset();
nit: you don't need this->

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:204: new
net::WrappedIOBuffer(static_cast<char*>(data) + pos));
Can we keep using IOBufferWithByteBuffer here? Yes, it will hold additional
reference, but it will guarantee that write_buffer is pointing to valid memory
block.

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:377:
pending_write_data_ = std::move(pending_write_data);
should we assign this before calling SendData? IIRC it doesn't complete
synchronously now, but what if it will change?

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.h (right):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.h:45:
std::vector<scoped_refptr<net::IOBuffer>> write_buffer_list;
My concern about using net::IOBuffer insted of IOBufferWithByteBuffer is that it
is theoretically possible to destroy ByteBuffer making memory backing IOBuffer
invalid.

[xunjieli, 2016-05-13 15:35:22]

Thanks for the review!

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:65:
this->jwrite_buffer_list.Reset();
On 2016/05/13 15:12:58, mef wrote:
> nit: you don't need this->

Done.

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:204: new
net::WrappedIOBuffer(static_cast<char*>(data) + pos));
On 2016/05/13 15:12:58, mef wrote:
> Can we keep using IOBufferWithByteBuffer here? Yes, it will hold additional
> reference, but it will guarantee that write_buffer is pointing to valid memory
> block.

But we have the same check on line 193. Isn't the check in the constructor of
IOBufferWithByteBuffer not needed then?

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.h (right):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.h:45:
std::vector<scoped_refptr<net::IOBuffer>> write_buffer_list;
On 2016/05/13 15:12:58, mef wrote:
> My concern about using net::IOBuffer insted of IOBufferWithByteBuffer is that
it
> is theoretically possible to destroy ByteBuffer making memory backing IOBuffer
> invalid.

But the Java ByteBuffer is not going to be destroyed/GC-ed since we retain a
global reference to it as |jwrite_buffer_list|, right?

[xunjieli, 2016-05-16 13:17:46]

Misha: friendly ping? If possible, I am hoping to get this into this week's dev
release. Thanks!

[mef, 2016-05-16 13:22:28]

On 2016/05/16 13:17:46, xunjieli wrote:
> Misha: friendly ping? If possible, I am hoping to get this into this week's
dev
> release. Thanks!

Sorry, looking.

[mef, 2016-05-16 13:54:18]

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:204: new
net::WrappedIOBuffer(static_cast<char*>(data) + pos));
On 2016/05/13 15:35:22, xunjieli wrote:
> On 2016/05/13 15:12:58, mef wrote:
> > Can we keep using IOBufferWithByteBuffer here? Yes, it will hold additional
> > reference, but it will guarantee that write_buffer is pointing to valid
memory
> > block.
> 
> But we have the same check on line 193. Isn't the check in the constructor of
> IOBufferWithByteBuffer not needed then?

My concern was that Java objects could theoretically get  released / destroyed
before WrappedIOBuffer.
I can't think of the plausible scenario though, so I'm ok with just adding
comment in PendingWriteData that write_buffer_list buffers point to memory owned
by buffers in jwrite_buffer_list.

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.h (right):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.h:45:
std::vector<scoped_refptr<net::IOBuffer>> write_buffer_list;
On 2016/05/13 15:35:22, xunjieli wrote:
> On 2016/05/13 15:12:58, mef wrote:
> > My concern about using net::IOBuffer insted of IOBufferWithByteBuffer is
that
> it
> > is theoretically possible to destroy ByteBuffer making memory backing
IOBuffer
> > invalid.
> 
> But the Java ByteBuffer is not going to be destroyed/GC-ed since we retain a
> global reference to it as |jwrite_buffer_list|, right?

Yeah, that's fine.

[xunjieli, 2016-05-16 14:11:12]

Patchset #5 (id:100001) has been deleted

[xunjieli, 2016-05-16 14:12:28]

Thanks for the review!

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right):

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:204: new
net::WrappedIOBuffer(static_cast<char*>(data) + pos));
On 2016/05/16 13:54:17, mef wrote:
> On 2016/05/13 15:35:22, xunjieli wrote:
> > On 2016/05/13 15:12:58, mef wrote:
> > > Can we keep using IOBufferWithByteBuffer here? Yes, it will hold
additional
> > > reference, but it will guarantee that write_buffer is pointing to valid
> memory
> > > block.
> > 
> > But we have the same check on line 193. Isn't the check in the constructor
of
> > IOBufferWithByteBuffer not needed then?
> 
> My concern was that Java objects could theoretically get  released / destroyed
> before WrappedIOBuffer.
> I can't think of the plausible scenario though, so I'm ok with just adding
> comment in PendingWriteData that write_buffer_list buffers point to memory
owned
> by buffers in jwrite_buffer_list.

Done.

https://codereview.chromium.org/1969893002/diff/60001/components/cronet/andro...
components/cronet/android/cronet_bidirectional_stream_adapter.cc:377:
pending_write_data_ = std::move(pending_write_data);
On 2016/05/13 15:12:58, mef wrote:
> should we assign this before calling SendData? IIRC it doesn't complete
> synchronously now, but what if it will change?

Done.

[mef, 2016-05-16 16:32:33]

lgtm

https://codereview.chromium.org/1969893002/diff/120001/net/spdy/bidirectional...
File net/spdy/bidirectional_stream_spdy_impl.h (right):

https://codereview.chromium.org/1969893002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.h:51: void SendvData(const
std::vector<scoped_refptr<IOBuffer>>& buffers,
#include <vector>

[xunjieli, 2016-05-16 18:15:59]

Thanks!

https://codereview.chromium.org/1969893002/diff/120001/net/spdy/bidirectional...
File net/spdy/bidirectional_stream_spdy_impl.h (right):

https://codereview.chromium.org/1969893002/diff/120001/net/spdy/bidirectional...
net/spdy/bidirectional_stream_spdy_impl.h:51: void SendvData(const
std::vector<scoped_refptr<IOBuffer>>& buffers,
On 2016/05/16 16:32:32, mef wrote:
> #include <vector>

Done.

[xunjieli, 2016-05-16 18:16:03]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-05-16 18:16:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1969893002/#ps140001
(title: "Add include vector")

[commit-bot: I haz the power, 2016-05-16 18:16:46]

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

[commit-bot: I haz the power, 2016-05-16 19:38:38]

Description was changed from

==========
Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

Save arguments passed in through WritevData directly instead
of a list of IOBufferWithByteBuffer.

This saves unnecessary array allocations to reconstruct Java
arguments for invoking onWriteCompleted.

BUG=611056
==========

to

==========
Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

Save arguments passed in through WritevData directly instead
of a list of IOBufferWithByteBuffer.

This saves unnecessary array allocations to reconstruct Java
arguments for invoking onWriteCompleted.

BUG=611056
==========

[commit-bot: I haz the power, 2016-05-16 19:38:39]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-05-16 19:40:52]

Description was changed from

==========
Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

Save arguments passed in through WritevData directly instead
of a list of IOBufferWithByteBuffer.

This saves unnecessary array allocations to reconstruct Java
arguments for invoking onWriteCompleted.

BUG=611056
==========

to

==========
Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

Save arguments passed in through WritevData directly instead
of a list of IOBufferWithByteBuffer.

This saves unnecessary array allocations to reconstruct Java
arguments for invoking onWriteCompleted.

BUG=611056

Committed: https://crrev.com/2328a268fb12d5b99c377be5548b055c0cec9c28
Cr-Commit-Position: refs/heads/master@{#393901}
==========

[commit-bot: I haz the power, 2016-05-16 19:40:53]

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