Avoid heap allocations in IPAddress

net/dns/mojo_host_struct_traits.h

Index: net/dns/mojo_host_struct_traits.h
diff --git a/net/dns/mojo_host_struct_traits.h b/net/dns/mojo_host_struct_traits.h
index 307e1b22ba8371aa75bb145abc95825e6270c523..bcd3dd0aceb5db61fb1ebdcc1c4f4869b0f976bf 100644
--- a/net/dns/mojo_host_struct_traits.h
+++ b/net/dns/mojo_host_struct_traits.h
@@ -50,8 +50,9 @@ struct StructTraits<net::interfaces::HostResolverRequestInfoDataView,
 
 template <>
 struct StructTraits<net::interfaces::IPEndPointDataView, net::IPEndPoint> {
-  static const std::vector<uint8_t>& address(const net::IPEndPoint& obj) {
-    return obj.address().bytes();
+  static const std::vector<uint8_t> address(const net::IPEndPoint& obj) {
+    return std::vector<uint8_t>(obj.address().bytes().begin(),

[eroman, 2017/05/12 23:25:05]

.BytesAsVector() ?

Although per earlier comment, I think this should just be changed to directly
create the IPAddress without the vector temporary.

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.
I don't think I understand this comment. The method wants to return a
vector<uint8_t> taking an IPAddress as an input. I don't think it's constructing
an IPAddress. I ping'd rdsmith about this change before I sent out the CL and he
thought the overhead of creating a new vector here was not a problem. But I
happy to do something better if you have a suggestion?

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

I don't know enough about mojo bindings to suggest how to accomplish this
better.

My questions are:
 (1) host_resolver_service.mojom represents |address| as an array<uint8>, which
tracked the previous implementation. To match the C++ code it feels like this
should either be using array<uint8, 16>, or having separate fields for the
bytes. I don't know much about the internal representation mojo uses and if
there is a better way to do fixed length arrays (although would still need a
length specifier too).

(2) Is inflating to a std::vector<> necessary?

I don't know the answers to these questions, but wonder if there is a more
direct adapter for the serialization that can be used.

I agree that it is reasonable to leave this to a follow-up, but think it is
worth annotating with a TODO if not addressed here. WDYT?

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

rdsmith: do you know enough about mojo to help out here?

[Randy Smith (Not in Mondays), 2017/05/17 18:53:58]

This is getting into implementation guts, and I'd like to defer to the expert. 
Yuzhu, can you comment on this issue?  Specifically, is there a way to specify a
fixed length array in mojo?  If not, is there any recommended procedure for
handling fixed length arrays?  I suppose we could break the bytes out
individually.

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

You could do say array<uint8, 16>

Not sure how that translates into code if it has any advantages. And you would
still need a field for the length anyway, so may not offer an advantage over
just making it variable sized as length + data.

[Randy Smith (Not in Mondays), 2017/05/17 20:52:54]

I don't think mojo has a type that translates into that.

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

It's worth keeping in mind that while the backing store is std::array<uint8_t,
16>, the std::vector here might be of a different length if the IP is an IPv4
address.

[yzshen1, 2017/05/18 22:32:47]

Sorry for missing this question.
As Eric said, you could use something like array<uint8, 16>.

It gets translate to std::vector<uint8_t>. The benefit is that Mojo will help
you to ensure that it has correct size: the sender side will DCHECK if the size
is wrong; the receiver side will verify the size and if it is wrong the whole
message will be regarded as invalid and pipe disconnected. So as a user, you
don't need to check the size when it is received.

[Ryan Hamilton, 2017/05/19 03:38:49]

I don't think that will work because sometimes the vector is of size 16 and
sometimes it's of size 4, depending on the address in question. So I can't make
the return type of this method std::array<uint8_t, 16> if that would imply mojo
would assume a length of 16. Do I understand that right? I don't suppose mojo
has an "array + length" type?

[yzshen1, 2017/05/19 16:04:03]

Please note that when I said array<uint8, 16>, it is the mojom IDL type, it is
mapped to std::vector<uint8_t> in C++, instead of std::array<>.

Mojo will validate that the vector serialized/deserialized is exactly of size
16.
I don't understand what you mean. mojo array always has the length information.
In mojo messages, the layout of array looks like this [byte_count,
element_count, element_0, element_1, ...]

It is converted to a std::vector, which has the length information.

+                                obj.address().bytes().end());
   }
   static uint16_t port(const net::IPEndPoint& obj) { return obj.port(); }