Avoid heap allocations in IPAddress

content/public/common/common_param_traits.cc

Index: content/public/common/common_param_traits.cc
diff --git a/content/public/common/common_param_traits.cc b/content/public/common/common_param_traits.cc
index 286e6621cc6212cf727ba636c44d4df4a0ba6ceb..149804d8119f5f9adcce333c4cb3cdbc89ddfce1 100644
--- a/content/public/common/common_param_traits.cc
+++ b/content/public/common/common_param_traits.cc
@@ -124,11 +124,13 @@ void ParamTraits<net::IPEndPoint>::Log(const param_type& p, std::string* l) {
 
 void ParamTraits<net::IPAddress>::GetSize(base::PickleSizer* s,
                                           const param_type& p) {
-  GetParamSize(s, p.bytes());
+  for (uint8_t byte : p.bytes())

[eroman, 2017/05/15 22:15:23]

This doesn't look right to me, are browser tests passing?

[Ryan Hamilton, 2017/05/17 18:26:41]

Yes, it looks like they are. I *think* This is correct. For example, let's look
at the implementation of this method for url::Origin:

void ParamTraits<url::Origin>::GetSize(base::PickleSizer* s,
                                       const param_type& p) {
  GetParamSize(s, p.unique());
  GetParamSize(s, p.scheme());
  GetParamSize(s, p.host());
  GetParamSize(s, p.port());
  GetParamSize(s, p.suborigin());
}

It simply calls GetParamSize on each individual member, which is just what I'm
doing here, right?

+    GetParamSize(s, byte);
 }
 
 void ParamTraits<net::IPAddress>::Write(base::Pickle* m, const param_type& p) {
-  WriteParam(m, p.bytes());
+  for (uint8_t byte : p.bytes())

[eroman, 2017/05/15 22:15:23]

Same here. Not sure how this works, given it is not aligned with the Read() call
below.

What I was suggesting was avoiding the conversion to std::vector<>, and just
constructing the values directly in the ParamTraits<net::IPAddress>.

For instance, you could see the std::vector<> one for inspiration:

https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?dr&l=479

If you don't want to do that change here, you can leave a TODO for it.

[Ryan Hamilton, 2017/05/17 18:26:41]

I *think* this works and seems correct to me. Here's the url::Origin
implementaion:

void ParamTraits<url::Origin>::Write(base::Pickle* m, const url::Origin& p) {
  WriteParam(m, p.unique());
  WriteParam(m, p.scheme());
  WriteParam(m, p.host());
  WriteParam(m, p.port());
  WriteParam(m, p.suborigin());
}

which, just calls WriteParam() on each field, which is basically what I'm doing
here. What do you think?

[eroman, 2017/05/17 20:48:16]

I don't see how this code can be correct, what am I missing?

For the size, it is calling GetParamSize() a variable number of times -->
p.bytes().size()

Each of these calls will add in a size of 4 bytes (not 1), since adding a byte
parameter appears to 32-bit align th data (see   ParamTraits<unsigned
char>::GetSize).

Write() does something equivalent.

But now ParamTraits<net::IPAddress>::Read() below does something totally
different -- it reads back the bytes using the param traits for std::vector<>,
which encodes the data as an Int for size, followed by the byte stream.

[Ryan Hamilton, 2017/05/17 23:14:04]

*facepalm*  Yes, you're right of course. I've switched back to constructing a
vector and using that. In the interest of being as efficient as possible, I
constructed a StackVector. But perhaps just using BytesAsVector() would be more
obvious. What do you think?

+    WriteParam(m, byte);
 }
 
 bool ParamTraits<net::IPAddress>::Read(const base::Pickle* m,