net: Change type of UploadData::elements from std::vector to ScopedVector

net: Change type of UploadData::elements from std::vector to ScopedVector

Using std::vector to hold UploadElement is bad for two reasons:
 1. It results in a lot of unnecessary copy of uploaded data.
 2. Appending new chunks may result in invalidating the pointer held by UploadBytesElementReader.

BUG=160028
TEST=git try
TBR=ananta@chromium.org, tony@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167611

[hashimoto, 2012-11-09 07:24:11]

mmenke@: Please review as the main reviewer.
jam@: Please review chrome/common/automation_messages.cc.

[mmenke, 2012-11-09 15:50:19]

LGTM

http://codereview.chromium.org/11275223/diff/10003/webkit/glue/resource_reque...
File webkit/glue/resource_request_body_unittest.cc (right):

http://codereview.chromium.org/11275223/diff/10003/webkit/glue/resource_reque...
webkit/glue/resource_request_body_unittest.cc:208:
ASSERT_EQ(upload->elements().size(), 8U);
While you're here, mind flipping the order of the parameters in the 7 asserts
like this one?

[jam, 2012-11-09 17:38:48]

https://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_...
File chrome/common/automation_messages.cc (right):

https://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_...
chrome/common/automation_messages.cc:225: struct
ParamTraits<ScopedVector<net::UploadElement> > {
can you add a param traits for ScopedVector that's templatized for the type,
i.e. similar to what we do for std::vector:
http://code.google.com/searchframe#OAMlx_jo-ck/src/ipc/ipc_message_utils.h&ex...

that way if anyone else wants to send a ScopedVector, they don't have to write
their own traits

[hashimoto, 2012-11-10 09:34:52]

Move the param traits to ipc_message_utils.h and added DISALLOW_COPY_AND_ASSIGN
to UploadElement.
PTAL

http://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_m...
File chrome/common/automation_messages.cc (right):

http://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_m...
chrome/common/automation_messages.cc:225: struct
ParamTraits<ScopedVector<net::UploadElement> > {
On 2012/11/09 17:38:48, John Abd-El-Malek wrote:
> can you add a param traits for ScopedVector that's templatized for the type,
> i.e. similar to what we do for std::vector:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/ipc/ipc_message_utils.h&ex...
> 
> that way if anyone else wants to send a ScopedVector, they don't have to write
> their own traits

Moved this to ipc_messge_utils.h.

I've switched from resize() to push_back() because it's safer when |size| is
broken.
It does not cost much because ScopedVector is holding pointers of elements so
that push_back() may not result in copying already pushed elements.

http://codereview.chromium.org/11275223/diff/10003/webkit/glue/resource_reque...
File webkit/glue/resource_request_body_unittest.cc (right):

http://codereview.chromium.org/11275223/diff/10003/webkit/glue/resource_reque...
webkit/glue/resource_request_body_unittest.cc:208:
ASSERT_EQ(upload->elements().size(), 8U);
On 2012/11/09 15:50:19, Matt Menke wrote:
> While you're here, mind flipping the order of the parameters in the 7 asserts
> like this one?

Done.

[jam, 2012-11-12 20:58:54]

http://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_m...
File chrome/common/automation_messages.cc (right):

http://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_m...
chrome/common/automation_messages.cc:225: struct
ParamTraits<ScopedVector<net::UploadElement> > {
On 2012/11/10 09:34:52, hashimoto wrote:
> On 2012/11/09 17:38:48, John Abd-El-Malek wrote:
> > can you add a param traits for ScopedVector that's templatized for the type,
> > i.e. similar to what we do for std::vector:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/ipc/ipc_message_utils.h&ex...
> > 
> > that way if anyone else wants to send a ScopedVector, they don't have to
write
> > their own traits
> 
> Moved this to ipc_messge_utils.h.
> 
> I've switched from resize() to push_back() because it's safer when |size| is
> broken.

I'm not sure what you mean. you mean when it comes in as very large?

the logic here should be the same as std::vector. if resize is not right to use,
then the other one isn't right either. but if it's so far been fine, then I'm
doubtful it needs to change for this one.

> It does not cost much because ScopedVector is holding pointers of elements so
> that push_back() may not result in copying already pushed elements.

http://codereview.chromium.org/11275223/diff/4007/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/11275223/diff/4007/ipc/ipc_message_utils.h#new...
ipc/ipc_message_utils.h:649: for (int i = 0; i < size; i++) {
you should do error checking just like std::vector does, i.e. the
 if (INT_MAX / sizeof(P) <= static_cast<size_t>(size))
 322       return false;	 323       return false;

[hashimoto, 2012-11-13 05:03:51]

http://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_m...
File chrome/common/automation_messages.cc (right):

http://codereview.chromium.org/11275223/diff/10003/chrome/common/automation_m...
chrome/common/automation_messages.cc:225: struct
ParamTraits<ScopedVector<net::UploadElement> > {
On 2012/11/12 20:58:54, John Abd-El-Malek wrote:
> On 2012/11/10 09:34:52, hashimoto wrote:
> > On 2012/11/09 17:38:48, John Abd-El-Malek wrote:
> > > can you add a param traits for ScopedVector that's templatized for the
type,
> > > i.e. similar to what we do for std::vector:
> > >
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/ipc/ipc_message_utils.h&ex...
> > > 
> > > that way if anyone else wants to send a ScopedVector, they don't have to
> write
> > > their own traits
> > 
> > Moved this to ipc_messge_utils.h.
> > 
> > I've switched from resize() to push_back() because it's safer when |size| is
> > broken.
> 
> I'm not sure what you mean. you mean when it comes in as very large?
> 
> the logic here should be the same as std::vector. if resize is not right to
use,
> then the other one isn't right either. but if it's so far been fine, then I'm
> doubtful it needs to change for this one.
I thought this size check was to avoid copying heavy structs again and again as
the vector getting bigger.
Since ScopedVector<P> is virtually a std::vector<P*> (i.e. it just copies
pointers when expanding, no possibility of copying heavy structs), I thought
this size check was not needed for ScopedVector. (Provided std::vector is
implemented correctly, calling push_back() n times is O(n), the same order as
calling resize() beforehand.)

Anyways, I don't have strong opinion about this and having this size check seems
good if we prefer consistency regarding this code is placed in
ipc_message_utils.h.
> 
> > It does not cost much because ScopedVector is holding pointers of elements
so
> > that push_back() may not result in copying already pushed elements.
>

http://codereview.chromium.org/11275223/diff/4007/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/11275223/diff/4007/ipc/ipc_message_utils.h#new...
ipc/ipc_message_utils.h:649: for (int i = 0; i < size; i++) {
On 2012/11/12 20:58:54, John Abd-El-Malek wrote:
> you should do error checking just like std::vector does, i.e. the
>  if (INT_MAX / sizeof(P) <= static_cast<size_t>(size))
>  322       return false;	 323       return false;

What is still unclear for me is that do we need to replace "sizeof(P)" here with
"sizeof(P*)" or not.
Since ScopedVector<P> is virtually a std::vector<P*>, perhaps INT_MAX/sizeof(P*)
might make more sense.

INT_MAX/sizeof(P) may work better in some cases if we want to avoid consuming
memory more than INT_MAX bytes (=2GB), but it does not work for some cases
including net::UploadElement because net::UploadElement has a std::vector as its
member and sizeof(std::vector<P>) is constant (i.e. sizeof(std::vector) cannot
know how much heap does the vector consume)
It means "INT_MAX > size*sizeof(P)" cannot guarantee that the total memory
consumed by the deserialized ScopedVector<P> not exceeding INT_MAX bytes when P
holds data on heap.

Anyways, I leave INT_MAX/sizeof(P) here for now.

[jam, 2012-11-13 16:09:23]

lgtm

[hashimoto, 2012-11-14 02:26:18]

TBR-ing ananta@ for src/chrome_frame, tony@ for src/webkit/glue

[commit-bot: I haz the power, 2012-11-14 02:29:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11275223/21

[commit-bot: I haz the power, 2012-11-14 06:35:55]

Change committed as 167611