Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881673002:
Avoid heap allocations in IPAddress (Closed)
|
Created:
3 years, 7 months ago by Ryan Hamilton Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, bnc+watch_chromium.org, darin (slow to review), net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid heap allocations in IPAddress
Use a helper class to represent the sequence of bytes in an IP address.
A vector<uint8_t> would be simpler but incurs heap allocation, so
IPAddressBytes uses a fixed size array.
IPAddress construction and destruction is showing up in cronet profiles
consuming roughly 5% of CPU. With this fix, it drops below .1%.
Also switch call sites which create IPAddress objects to avoid using std::vector and instead use base::StackVector.
Review-Url: https://codereview.chromium.org/2881673002
Cr-Commit-Position: refs/heads/master@{#474780}
Committed: https://chromium.googlesource.com/chromium/src/+/c6635f4e242cd2236f85a2b84cc42dfb945ad5d2
Patch Set 1 #Patch Set 2 : Fix #Patch Set 3 : remoting #Patch Set 4 : Fixes #Patch Set 5 : Comments #Patch Set 6 : Fix #Patch Set 7 : More fixes #
Total comments: 53
Patch Set 8 : Address comments #
Total comments: 10
Patch Set 9 : Address comments #Patch Set 10 : DCHECK #
Total comments: 2
Patch Set 11 : IPC fixed #Patch Set 12 : Resize #
Total comments: 19
Patch Set 13 : Fix more comments and use StackVector more #
Total comments: 10
Patch Set 14 : Almost there #Patch Set 15 : Done #
Total comments: 13
Patch Set 16 : more fixes #Patch Set 17 : Address comments #
Total comments: 2
Patch Set 18 : Rename and Assign #
Total comments: 2
Patch Set 19 : Rebase #Patch Set 20 : Fix Assign #
Total comments: 2
Patch Set 21 : Fix #Patch Set 22 : copys #
Total comments: 2
Patch Set 23 : Copyies #Patch Set 24 : New constructor #Messages
Total messages: 95 (53 generated)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Avoid heap allocations in IPAddress Use a helper class to represent the sequence of bytes in an IP address. A vector<uint8_t> would be simpler but incurs heap allocation, so IPAddressBytes uses a fixed size array ========== to ========== Avoid heap allocations in IPAddress Use a helper class to represent the sequence of bytes in an IP address. A vector<uint8_t> would be simpler but incurs heap allocation, so IPAddressBytes uses a fixed size array. IPAddress construction and destruction is showing up in cronet profiles consuming roughly 5% of CPU. With this fix, it drops below .1%. ==========
rch@chromium.org changed reviewers: + eroman@chromium.org
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Some design-level questions before I dive into the details... (1) Did you consider using base/containers/stack_container.h ? This has the advantage of preserving the current semantics (using an std::vector<>, but with the optimization that you can set the initial stack capacity). I think this would be the easiest way to wire things up, and also avoids some copies that it looks like this introduces in going from IPAddressBytes to std::vector<>. (2) What metrics are you using to measure the impact of this change? While I am very amenable to the idea that this is a net improvement, I would like to understand: * How much does the extra size of IPAddress hurt performance? (i.e. copying/iterating in say a std::vector<IPAddress> is more expensive). * Is the choice of 16-bytes for stack-allocated size intentional, or out of convenience? Notably, if IPv6 addresses are infrequently used we may be better off defaulting to only 4 bytes reserved, as this is the minimum (and possibly the average) needed. * Do you have any metrics on the total increase in memory? We cache IPAddress in some places, such as the HostResolver cache. Modulo heap fragmentation for the extra allocs, we may now be consuming more memory. I suspect memory usage won't be measurably worse, but wanted to ask. Certainly at 4 bytes reserved per IPAddress I think it would clearly be a win. (3) Does your design require for IPAddress to be only 4 bytes or 16 bytes? Unfortunately I believe there is code which violates that assumption, since in truth it ends up being used more like a sockaddr than strictly an "IP address", in order to re-use other socket APIs. In particular, I recall being horrified to see that bluetooth addresses (48-bits) were being smuggled into an IPAddress to then implement bluetooth sockets :( So I wouldn't be shocked if the versatility of having a std::vector<> for years has caused this to become canonicalized as part of the API.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/05/12 18:56:53, eroman wrote: > Some design-level questions before I dive into the details... > > (1) Did you consider using base/containers/stack_container.h ? This has the > advantage of preserving the current semantics (using an std::vector<>, but with > the optimization that you can set the initial stack capacity). I think this > would be the easiest way to wire things up, and also avoids some copies that it > looks like this introduces in going from IPAddressBytes to std::vector<>. I did! I had quite a hard time understanding the guarantees of this class and eventually decided to roll my own. But I could use that if you prefer. It would, however, consume more memory than this approach. > (2) What metrics are you using to measure the impact of this change? We're running manual tests which POST and GET 100Mb files. > While I am > very amenable to the idea that this is a net improvement, I would like to > understand: > > * How much does the extra size of IPAddress hurt performance? (i.e. > copying/iterating in say a std::vector<IPAddress> is more expensive). I believe this change makes IPAddress use *less* memory, not more. The overhead of a vector is 16 bytes, according to what I read on the internet. It has: * begin and end pointers * capacity * allocator So I believe this is simply a memory win. > * Is the choice of 16-bytes for stack-allocated size intentional, or out of > convenience? Notably, if IPv6 addresses are infrequently used we may be better > off defaulting to only 4 bytes reserved, as this is the minimum (and possibly > the average) needed. IPv6 is increasingly common, especially on mobile where it is virtually ubiquitous. It would be a bummer to burn CPU for those users, so I'd be very sad about having to do an allocation in that case. https://www.google.com/intl/en/ipv6/statistics.html IPv6 is 32% in the US, for example. > * Do you have any metrics on the total increase in memory? We cache IPAddress > in some places, such as the HostResolver cache. Modulo heap fragmentation for > the extra allocs, we may now be consuming more memory. I suspect memory usage > won't be measurably worse, but wanted to ask. Certainly at 4 bytes reserved per > IPAddress I think it would clearly be a win. Pretty sure this is a net memory win because of vector overhead. > (3) Does your design require for IPAddress to be only 4 bytes or 16 bytes? > Unfortunately I believe there is code which violates that assumption, since in > truth it ends up being used more like a sockaddr than strictly an "IP address", > in order to re-use other socket APIs. In particular, I recall being horrified to > see that bluetooth addresses (48-bits) were being smuggled into an IPAddress to > then implement bluetooth sockets :( So I wouldn't be shocked if the versatility > of having a std::vector<> for years has caused this to become canonicalized as > part of the API. *blink* It doesn't require to be 4 or 16, but it does require it to be <= 16. That being said, IPAddress.IsValid() (before and after my change) returns false if the size is not 4 or 16. So long as 48 bits is *less* than 16 bytes, this will work fine.
Thanks for the thoughtful comments! Also, happy to chat in person if that'd be more efficient...
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/12 19:11:52, Ryan Hamilton wrote: > On 2017/05/12 18:56:53, eroman wrote: > > * How much does the extra size of IPAddress hurt performance? (i.e. > > copying/iterating in say a std::vector<IPAddress> is more expensive). > > I believe this change makes IPAddress use *less* memory, not more. The overhead > of a vector is 16 bytes, according to what I read on the internet. It has: > * begin and end pointers > * capacity > * allocator > > So I believe this is simply a memory win. I did a quick test. With this CL, sizeof(IPAddress) is 17 (1 byte for the size and 16 bytes for the data). Without this CL, sizeof(IPAddress) on my linux box (after constructing either a v4 or v6 add) is 56?! (That doesn't count the data on the heap). Other platforms will vary, of course.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/12 21:52:51, Ryan Hamilton wrote: > On 2017/05/12 19:11:52, Ryan Hamilton wrote: > > On 2017/05/12 18:56:53, eroman wrote: > > > * How much does the extra size of IPAddress hurt performance? (i.e. > > > copying/iterating in say a std::vector<IPAddress> is more expensive). > > > > I believe this change makes IPAddress use *less* memory, not more. The > overhead > > of a vector is 16 bytes, according to what I read on the internet. It has: > > * begin and end pointers > > * capacity > > * allocator > > > > So I believe this is simply a memory win. > > I did a quick test. > > With this CL, sizeof(IPAddress) is 17 (1 byte for the size and 16 bytes for the > data). > > Without this CL, sizeof(IPAddress) on my linux box (after constructing either a > v4 or v6 add) is 56?! (That doesn't count the data on the heap). > > Other platforms will vary, of course. Oh, duh. That's a debug build Without this CL in a release build sizeof(IPAddress) is 24 bytes (which is the size of 3 64-bit pointers). (+ 4 or 16 for the data on the heap, of course)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sounds like a good improvement, thanks! https://codereview.chromium.org/2881673002/diff/110001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/110001/content/public/common/... content/public/common/common_param_traits.cc:127: GetParamSize(s, p.BytesAsVector()); Can you improve the efficiency of this? (Constructs a temporary vector, only to get the size). https://codereview.chromium.org/2881673002/diff/110001/content/public/common/... content/public/common/common_param_traits.cc:131: WriteParam(m, p.BytesAsVector()); Same here. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:141: bool operator<(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs) { std::array<> already defines these operators (as lexicographic compares, which is what we want), so you can just forward to them. As in: return lhs < rhs; return lhs > rhs; return lhs == rhs; https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:181: IPAddress::IPAddress(const std::vector<uint8_t>& address) { I am a bit worried that we have codepaths that may pass variable sized addresses, that will now result in memory errors. Am I being overly paranoid? Should we throw in a CHECK() at the entry points which take variable sized data? (i.e. here and the other ctor). We can leave things like push_back() as DCHECK(). https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:183: ip_address_.push_back(val); may want to do a single memcpy(). Or perhaps code generator is smart enough to optimize the loop. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:190: ip_address_.push_back(address[i]); Same comments as above. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:258: for (auto x : ip_address_) { Fancy. So this works because of the begin() and end() methods? https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:276: std::swap(number, ip_address_); Should probably just change this to assignment now: ip_address_ = number; std::swap() is worse now, since we need to copy ip_address_ back into number_ too. On a separate note, we should probably just change the contract so ip_address_ is cleared on failure, to avoid needing this temporary at all. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:465: std::vector<uint8_t> all_ones(mask.size(), 0xFF); If IPAddress were mutable (i.e. merge IPAddressBytes into it), patterns like this will be a bit simpler. (Can create a stack-based IPAddress and manipulate it directly, without the temporary vector<>). https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:55: const uint8_t* end() const { return &(data()[size_]); } return data() + size; https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:58: uint8_t& back() { return bytes_[size_ - 1]; } DCHECK(!empty()) ? https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:59: const uint8_t& back() const { return bytes_[size_ - 1]; } ditto. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:68: uint8_t& operator[](size_t pos) { return bytes_[pos]; } DCHECK that it is in range? https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:72: friend bool operator>(IPAddressBytes lhs, IPAddressBytes rhs); const IPAddressBytes& https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:78: // or kIPv6AddressSize. or 0. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:79: uint8_t size_; [optional] I suggest putting this after |bytes_|. My rationale is it doesn't hurt the structure padding, and if we have buffer overflows the first byte we will corrupt is the size, which increases the odds of something going wrong deterministically. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:194: IPAddressBytes ip_address_; Was IPAddressBytes made a separate class (as opposed to merging into IPAddress) just to preserve the immutability of IPAddress (whereas IPAddressBytes is mutable)? I don't feel too strongly either way. I think it is fine to flatting it all into IPAddress -- will also simplify places where we create temporary buffers just to initialize an IPAddress (rather than being able to write directly into its memory). https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:199: bool operator>(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs); const IPAddressBytes&, or we will create unnecessary copies. https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), .BytesAsVector() ? Although per earlier comment, I think this should just be changed to directly create the IPAddress without the vector temporary. https://codereview.chromium.org/2881673002/diff/110001/net/interfaces/ip_addr... File net/interfaces/ip_address_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/interfaces/ip_addr... net/interfaces/ip_address_struct_traits.h:16: return std::vector<uint8_t>(ip_address.bytes().begin(), Ditto.
https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:40: DCHECK_GT(16u, size); DCHECK_LE(size, 16); Either way, need equality in there.
https://codereview.chromium.org/2881673002/diff/110001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/110001/content/public/common/... content/public/common/common_param_traits.cc:127: GetParamSize(s, p.BytesAsVector()); On 2017/05/12 23:25:04, eroman wrote: > Can you improve the efficiency of this? > (Constructs a temporary vector, only to get the size). Good point. Done. https://codereview.chromium.org/2881673002/diff/110001/content/public/common/... content/public/common/common_param_traits.cc:131: WriteParam(m, p.BytesAsVector()); On 2017/05/12 23:25:04, eroman wrote: > Same here. Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:141: bool operator<(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs) { On 2017/05/12 23:25:04, eroman wrote: > std::array<> already defines these operators (as lexicographic compares, which > is what we want), so you can just forward to them. As in: > > return lhs < rhs; > return lhs > rhs; > return lhs == rhs; Ah! So I implemented this initially, and while it compiles, it's subtly wrong. (There's even a test which fails). The std::array operators look at all the elements in the array, but IPAddressByte operators only look at the elements in [0, size_). https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:181: IPAddress::IPAddress(const std::vector<uint8_t>& address) { On 2017/05/12 23:25:05, eroman wrote: > I am a bit worried that we have codepaths that may pass variable sized > addresses, that will now result in memory errors. > > Am I being overly paranoid? Should we throw in a CHECK() at the entry points > which take variable sized data? (i.e. here and the other ctor). We can leave > things like push_back() as DCHECK(). Fair enough! I added a CHECK() in the IPAddressBytes constructor which both these call into now. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:183: ip_address_.push_back(val); On 2017/05/12 23:25:05, eroman wrote: > may want to do a single memcpy(). Or perhaps code generator is smart enough to > optimize the loop. Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:190: ip_address_.push_back(address[i]); On 2017/05/12 23:25:04, eroman wrote: > Same comments as above. Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:258: for (auto x : ip_address_) { On 2017/05/12 23:25:04, eroman wrote: > Fancy. So this works because of the begin() and end() methods? Right?! Every once in a while, C++ doesn't suck. There are also a TON of places where IPAddress.bytes() is used in a similar context and works magically. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:276: std::swap(number, ip_address_); On 2017/05/12 23:25:05, eroman wrote: > Should probably just change this to assignment now: > ip_address_ = number; > > std::swap() is worse now, since we need to copy ip_address_ back into number_ > too. Done. > On a separate note, we should probably just change the contract so ip_address_ > is cleared on failure, to avoid needing this temporary at all. TODO added. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:465: std::vector<uint8_t> all_ones(mask.size(), 0xFF); On 2017/05/12 23:25:05, eroman wrote: > If IPAddress were mutable (i.e. merge IPAddressBytes into it), patterns like > this will be a bit simpler. (Can create a stack-based IPAddress and manipulate > it directly, without the temporary vector<>). How 'bout we do the flattening in a follow up since I suspect that's going to change quite a number of places. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:40: DCHECK_GT(16u, size); On 2017/05/12 23:27:02, eroman wrote: > DCHECK_LE(size, 16); > > Either way, need equality in there. Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:55: const uint8_t* end() const { return &(data()[size_]); } On 2017/05/12 23:25:05, eroman wrote: > return data() + size; Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:58: uint8_t& back() { return bytes_[size_ - 1]; } On 2017/05/12 23:25:05, eroman wrote: > DCHECK(!empty()) ? Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:59: const uint8_t& back() const { return bytes_[size_ - 1]; } On 2017/05/12 23:25:05, eroman wrote: > ditto. Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:68: uint8_t& operator[](size_t pos) { return bytes_[pos]; } On 2017/05/12 23:25:05, eroman wrote: > DCHECK that it is in range? Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:72: friend bool operator>(IPAddressBytes lhs, IPAddressBytes rhs); On 2017/05/12 23:25:05, eroman wrote: > const IPAddressBytes& Done. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:78: // or kIPv6AddressSize. On 2017/05/12 23:25:05, eroman wrote: > or 0. Good point! https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:79: uint8_t size_; On 2017/05/12 23:25:05, eroman wrote: > [optional] I suggest putting this after |bytes_|. My rationale is it doesn't > hurt the structure padding, and if we have buffer overflows the first byte we > will corrupt is the size, which increases the odds of something going wrong > deterministically. Ooh, clever. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:194: IPAddressBytes ip_address_; On 2017/05/12 23:25:05, eroman wrote: > Was IPAddressBytes made a separate class (as opposed to merging into IPAddress) > just to preserve the immutability of IPAddress (whereas IPAddressBytes is > mutable)? > > I don't feel too strongly either way. I think it is fine to flatting it all into > IPAddress -- will also simplify places where we create temporary buffers just to > initialize an IPAddress (rather than being able to write directly into its > memory). The mutability was part of it. I also wanted to, as much as possible, isolate the API change from the implementation change. https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.h#... net/base/ip_address.h:199: bool operator>(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs); On 2017/05/12 23:25:05, eroman wrote: > const IPAddressBytes&, or we will create unnecessary copies. Done. https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/12 23:25:05, eroman wrote: > .BytesAsVector() ? Done. > Although per earlier comment, I think this should just be changed to directly > create the IPAddress without the vector temporary. 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? https://codereview.chromium.org/2881673002/diff/110001/net/interfaces/ip_addr... File net/interfaces/ip_address_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/interfaces/ip_addr... net/interfaces/ip_address_struct_traits.h:16: return std::vector<uint8_t>(ip_address.bytes().begin(), On 2017/05/12 23:25:05, eroman wrote: > Ditto. Done.
On 2017/05/12 19:11:52, Ryan Hamilton wrote: > On 2017/05/12 18:56:53, eroman wrote: > > Some design-level questions before I dive into the details... > > > > (1) Did you consider using base/containers/stack_container.h ? This has the > > advantage of preserving the current semantics (using an std::vector<>, but > with > > the optimization that you can set the initial stack capacity). I think this > > would be the easiest way to wire things up, and also avoids some copies that > it > > looks like this introduces in going from IPAddressBytes to std::vector<>. > > I did! I had quite a hard time understanding the guarantees of this class and > eventually decided to roll my own. But I could use that if you prefer. It would, > however, consume more memory than this approach. > > > (2) What metrics are you using to measure the impact of this change? > > We're running manual tests which POST and GET 100Mb files. > > > While I am > > very amenable to the idea that this is a net improvement, I would like to > > understand: > > > > * How much does the extra size of IPAddress hurt performance? (i.e. > > copying/iterating in say a std::vector<IPAddress> is more expensive). > > I believe this change makes IPAddress use *less* memory, not more. The overhead > of a vector is 16 bytes, according to what I read on the internet. It has: > * begin and end pointers > * capacity > * allocator > > So I believe this is simply a memory win. > > > * Is the choice of 16-bytes for stack-allocated size intentional, or out of > > convenience? Notably, if IPv6 addresses are infrequently used we may be better > > off defaulting to only 4 bytes reserved, as this is the minimum (and possibly > > the average) needed. > > IPv6 is increasingly common, especially on mobile where it is virtually > ubiquitous. > It would be a bummer to burn CPU for those users, so I'd be very sad about > having to do an allocation in that case. > > https://www.google.com/intl/en/ipv6/statistics.html > > IPv6 is 32% in the US, for example. > > > * Do you have any metrics on the total increase in memory? We cache > IPAddress > > in some places, such as the HostResolver cache. Modulo heap fragmentation for > > the extra allocs, we may now be consuming more memory. I suspect memory usage > > won't be measurably worse, but wanted to ask. Certainly at 4 bytes reserved > per > > IPAddress I think it would clearly be a win. > > Pretty sure this is a net memory win because of vector overhead. > > > (3) Does your design require for IPAddress to be only 4 bytes or 16 bytes? > > Unfortunately I believe there is code which violates that assumption, since in > > truth it ends up being used more like a sockaddr than strictly an "IP > address", > > in order to re-use other socket APIs. In particular, I recall being horrified > to > > see that bluetooth addresses (48-bits) were being smuggled into an IPAddress > to > > then implement bluetooth sockets :( So I wouldn't be shocked if the > versatility > > of having a std::vector<> for years has caused this to become canonicalized as > > part of the API. > > *blink* > > It doesn't require to be 4 or 16, but it does require it to be <= 16. That being > said, > IPAddress.IsValid() (before and after my change) returns false if the size is > not 4 > or 16. So long as 48 bits is *less* than 16 bytes, this will work fine. Worth noting that unix domain paths are smuggled in sockaddrs in some places... I hope they're not stashed in IPAddresses as well, but I am not sure of that.
https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:141: bool operator<(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs) { On 2017/05/13 13:20:46, Ryan Hamilton wrote: > On 2017/05/12 23:25:04, eroman wrote: > > std::array<> already defines these operators (as lexicographic compares, which > > is what we want), so you can just forward to them. As in: > > > > return lhs < rhs; > > return lhs > rhs; > > return lhs == rhs; > > Ah! So I implemented this initially, and while it compiles, it's subtly wrong. > (There's even a test which fails). The std::array operators look at all the > elements in the array, but IPAddressByte operators only look at the elements in > [0, size_). Glad that there is test coverage to catch this! https://codereview.chromium.org/2881673002/diff/110001/net/base/ip_address.cc... net/base/ip_address.cc:465: std::vector<uint8_t> all_ones(mask.size(), 0xFF); On 2017/05/13 13:20:47, Ryan Hamilton wrote: > On 2017/05/12 23:25:05, eroman wrote: > > If IPAddress were mutable (i.e. merge IPAddressBytes into it), patterns like > > this will be a bit simpler. (Can create a stack-based IPAddress and manipulate > > it directly, without the temporary vector<>). > > How 'bout we do the flattening in a follow up since I suspect that's going to > change quite a number of places. sgtm https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/13 13:20:47, Ryan Hamilton wrote: > On 2017/05/12 23:25:05, eroman wrote: > > .BytesAsVector() ? > > Done. > > > Although per earlier comment, I think this should just be changed to directly > > create the IPAddress without the vector temporary. > > 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? 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? https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... content/public/common/common_param_traits.cc:127: for (uint8_t byte : p.bytes()) This doesn't look right to me, are browser tests passing? https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... content/public/common/common_param_traits.cc:132: for (uint8_t byte : p.bytes()) 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. https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.cc... net/base/ip_address.cc:140: memcpy(bytes_.data(), data, data_len); If you want to avoid relying on undefined behavior, unfortunately you need to special case the data_len == 0 case, because the C definition of memcpy() is stupid, and nullptr parameters lead to undefined behavior (even if length is zero). Go figure. https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.h#... net/base/ip_address.h:40: void resize(size_t size) { I presume this exists to make existing code (which was using std::vector<>) continue to work without changes? Does it then need to adhere to the same contract as std::vector::resize? In particular: x.resize(0); x.resize(16); Previously this would result in the address being all zeros. Whereas now the address bytes have whatever value they previously had. My suggestion is to rename this to "Resize()" if you want it to behave differently. Otherwise if there are lots of callers of resize() and you need to preserve that API, then make it match by zeroing out the new bytes lest this leads to subtle issues.
@mmenke: I don't think the unix domain sockets code is reliant on that (it passes a string around internally), from my quick perusal of it. FTR, some of the bluetooth shenanigans can be seen here: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_socket_win.cc... I am generally concerned there might be other abuses of the layering, but the CHECK() should help to surface those.
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rch@chromium.org changed reviewers: + rdsmith@chromium.org
rdsmith: a question for you about mojo https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/15 22:15:23, eroman wrote: > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > On 2017/05/12 23:25:05, eroman wrote: > > > .BytesAsVector() ? > > > > Done. > > > > > Although per earlier comment, I think this should just be changed to > directly > > > create the IPAddress without the vector temporary. > > > > 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? > > 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? rdsmith: do you know enough about mojo to help out here? https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... content/public/common/common_param_traits.cc:127: for (uint8_t byte : p.bytes()) On 2017/05/15 22:15:23, eroman wrote: > This doesn't look right to me, are browser tests passing? 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? https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... content/public/common/common_param_traits.cc:132: for (uint8_t byte : p.bytes()) On 2017/05/15 22:15:23, eroman wrote: > 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. 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? https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.cc... net/base/ip_address.cc:140: memcpy(bytes_.data(), data, data_len); On 2017/05/15 22:15:23, eroman wrote: > If you want to avoid relying on undefined behavior, unfortunately you need to > special case the data_len == 0 case, because the C definition of memcpy() is > stupid, and nullptr parameters lead to undefined behavior (even if length is > zero). Go figure. Done. https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/130001/net/base/ip_address.h#... net/base/ip_address.h:40: void resize(size_t size) { On 2017/05/15 22:15:23, eroman wrote: > I presume this exists to make existing code (which was using std::vector<>) > continue to work without changes? Yeah, specifically the code in ParseIPLiteralToBytes which passes in the array to url::IPv6AddressToNumber or url::IPv4AddressToNumber, which works on a vanilla unsigned char[]. > Does it then need to adhere to the same > contract as std::vector::resize? > > In particular: > > x.resize(0); > x.resize(16); > > Previously this would result in the address being all zeros. Whereas now the > address bytes have whatever value they previously had. > > My suggestion is to rename this to "Resize()" if you want it to behave > differently. Otherwise if there are lots of callers of resize() and you need to > preserve that API, then make it match by zeroing out the new bytes lest this > leads to subtle issues. Done.
rdsmith@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/17 18:26:41, Ryan Hamilton wrote: > On 2017/05/15 22:15:23, eroman wrote: > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > On 2017/05/12 23:25:05, eroman wrote: > > > > .BytesAsVector() ? > > > > > > Done. > > > > > > > Although per earlier comment, I think this should just be changed to > > directly > > > > create the IPAddress without the vector temporary. > > > > > > 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? > > > > 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? > > rdsmith: do you know enough about mojo to help out here? 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.
https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/17 18:53:58, Randy Smith (Not in Mondays) wrote: > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > On 2017/05/15 22:15:23, eroman wrote: > > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > > On 2017/05/12 23:25:05, eroman wrote: > > > > > .BytesAsVector() ? > > > > > > > > Done. > > > > > > > > > Although per earlier comment, I think this should just be changed to > > > directly > > > > > create the IPAddress without the vector temporary. > > > > > > > > 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? > > > > > > 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? > > > > rdsmith: do you know enough about mojo to help out here? > > 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. 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. https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... content/public/common/common_param_traits.cc:132: for (uint8_t byte : p.bytes()) On 2017/05/17 18:26:41, Ryan Hamilton wrote: > On 2017/05/15 22:15:23, eroman wrote: > > 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. > > 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? 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. https://codereview.chromium.org/2881673002/diff/170001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/170001/net/base/ip_address.h#... net/base/ip_address.h:39: // of the underlying array. Zeros out all elements in [0, |size|). This is not the same way that std::vector<>::resize() works. If changing the behavior, why not rename the function to Resize() as well?
https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/17 20:48:16, eroman wrote: > On 2017/05/17 18:53:58, Randy Smith (Not in Mondays) wrote: > > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > > On 2017/05/15 22:15:23, eroman wrote: > > > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > > > On 2017/05/12 23:25:05, eroman wrote: > > > > > > .BytesAsVector() ? > > > > > > > > > > Done. > > > > > > > > > > > Although per earlier comment, I think this should just be changed to > > > > directly > > > > > > create the IPAddress without the vector temporary. > > > > > > > > > > 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? > > > > > > > > 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? > > > > > > rdsmith: do you know enough about mojo to help out here? > > > > 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. > > You could do say array<uint8, 16> I don't think mojo has a type that translates into that. > 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.
https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/17 20:52:54, Randy Smith (Not in Mondays) wrote: > On 2017/05/17 20:48:16, eroman wrote: > > On 2017/05/17 18:53:58, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > > > On 2017/05/15 22:15:23, eroman wrote: > > > > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > > > > On 2017/05/12 23:25:05, eroman wrote: > > > > > > > .BytesAsVector() ? > > > > > > > > > > > > Done. > > > > > > > > > > > > > Although per earlier comment, I think this should just be changed to > > > > > directly > > > > > > > create the IPAddress without the vector temporary. > > > > > > > > > > > > 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? > > > > > > > > > > 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? > > > > > > > > rdsmith: do you know enough about mojo to help out here? > > > > > > 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. > > > > You could do say array<uint8, 16> > > I don't think mojo has a type that translates into that. > > > 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. > 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. https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/130001/content/public/common/... content/public/common/common_param_traits.cc:132: for (uint8_t byte : p.bytes()) On 2017/05/17 20:48:16, eroman wrote: > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > On 2017/05/15 22:15:23, eroman wrote: > > > 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. > > > > 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? > > 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. *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?
https://codereview.chromium.org/2881673002/diff/170001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/170001/net/base/ip_address.h#... net/base/ip_address.h:39: // of the underlying array. Zeros out all elements in [0, |size|). On 2017/05/17 20:48:16, eroman wrote: > This is not the same way that std::vector<>::resize() works. > If changing the behavior, why not rename the function to Resize() as well? Ah, ok. Renamed to Resize().
lgtm! https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... content/public/common/common_param_traits.cc:128: base::StackVector<uint8_t, 16> bytes; Better thanks! https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... content/public/common/common_param_traits.cc:145: std::vector<uint8_t> bytes; For symmetry with GetSize() and Write(), can you change this to a StackVector<> as well? https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.cc... net/base/ip_address.cc:25: bool IPAddressPrefixCheck(const net::IPAddress::IPAddressBytes& ip_address, optional: Enclose this anonymous namespace within namespace net and remove these prefixes. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.cc... net/base/ip_address.cc:140: if (data_len > 0) optional: std::copy_n(data, data_len, bytes_.data()); (really just a side-comment, as I know I suggested memcpy earlier, which probably generates better code) https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:27: // IPAddressBytes uses a fixed size array nits: end with period. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:39: // of the underlying array. or zero-initialize the bytes. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:85: friend bool operator>(const IPAddressBytes& lhs, const IPAddressBytes& rhs); optional: Instead of friending, what about defining the operators as public member functions of IPAddressBytes ? Probably saves a few extra lines. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:184: // Returns the underlying bytes as a vector. optional: Add a comment discouraging use of this function? https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:207: // IPv4 addresses will have length kIPv4AddressSize, whereas IPv6 address optional: I think this comment can be removed, since this is a documented detail of IPAddressBytes now.
https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/17 18:53:58, Randy Smith (Not in Mondays) wrote: > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > On 2017/05/15 22:15:23, eroman wrote: > > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > > On 2017/05/12 23:25:05, eroman wrote: > > > > > .BytesAsVector() ? > > > > > > > > Done. > > > > > > > > > Although per earlier comment, I think this should just be changed to > > > directly > > > > > create the IPAddress without the vector temporary. > > > > > > > > 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? > > > > > > 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? > > > > rdsmith: do you know enough about mojo to help out here? > > 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. 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.
https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/18 22:32:47, yzshen1 wrote: > On 2017/05/17 18:53:58, Randy Smith (Not in Mondays) wrote: > > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > > On 2017/05/15 22:15:23, eroman wrote: > > > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > > > On 2017/05/12 23:25:05, eroman wrote: > > > > > > .BytesAsVector() ? > > > > > > > > > > Done. > > > > > > > > > > > Although per earlier comment, I think this should just be changed to > > > > directly > > > > > > create the IPAddress without the vector temporary. > > > > > > > > > > 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? > > > > > > > > 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? > > > > > > rdsmith: do you know enough about mojo to help out here? > > > > 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. > > 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. 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? https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... content/public/common/common_param_traits.cc:145: std::vector<uint8_t> bytes; On 2017/05/18 18:43:17, eroman wrote: > For symmetry with GetSize() and Write(), can you change this to a StackVector<> > as well? Done. Though in order to do this, I needed to add a new IPAddress constructor which takes a StackVector. But that seems like a good idea. As a result, I replaced a number of calls to the existing std::vector constructor with this new StackVector constructor. Does that sound good to you? PTAL. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.cc... net/base/ip_address.cc:25: bool IPAddressPrefixCheck(const net::IPAddress::IPAddressBytes& ip_address, On 2017/05/18 18:43:17, eroman wrote: > optional: Enclose this anonymous namespace within namespace net and remove these > prefixes. Done. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.cc... net/base/ip_address.cc:140: if (data_len > 0) On 2017/05/18 18:43:17, eroman wrote: > optional: std::copy_n(data, data_len, bytes_.data()); > (really just a side-comment, as I know I suggested memcpy earlier, which > probably generates better code) Oh, much cleaner. Done. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:27: // IPAddressBytes uses a fixed size array On 2017/05/18 18:43:17, eroman wrote: > nits: end with period. Done. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:39: // of the underlying array. On 2017/05/18 18:43:17, eroman wrote: > or zero-initialize the bytes. Done. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:85: friend bool operator>(const IPAddressBytes& lhs, const IPAddressBytes& rhs); On 2017/05/18 18:43:17, eroman wrote: > optional: Instead of friending, what about defining the operators as public > member functions of IPAddressBytes ? Probably saves a few extra lines. Why did I not do that at first?! Done. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:184: // Returns the underlying bytes as a vector. On 2017/05/18 18:43:17, eroman wrote: > optional: Add a comment discouraging use of this function? Done. https://codereview.chromium.org/2881673002/diff/210001/net/base/ip_address.h#... net/base/ip_address.h:207: // IPv4 addresses will have length kIPv4AddressSize, whereas IPv6 address On 2017/05/18 18:43:17, eroman wrote: > optional: I think this comment can be removed, since this is a documented detail > of IPAddressBytes now. Good point. Done.
https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:54: return std::vector<uint8_t>(obj.address().bytes().begin(), On 2017/05/19 03:38:49, Ryan Hamilton wrote: > On 2017/05/18 22:32:47, yzshen1 wrote: > > On 2017/05/17 18:53:58, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > > > On 2017/05/15 22:15:23, eroman wrote: > > > > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > > > > On 2017/05/12 23:25:05, eroman wrote: > > > > > > > .BytesAsVector() ? > > > > > > > > > > > > Done. > > > > > > > > > > > > > Although per earlier comment, I think this should just be changed to > > > > > directly > > > > > > > create the IPAddress without the vector temporary. > > > > > > > > > > > > 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? > > > > > > > > > > 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? > > > > > > > > rdsmith: do you know enough about mojo to help out here? > > > > > > 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. > > > > 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. > > 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? 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 suppose mojo has an "array + length" type? 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.
still lgtm https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... content/public/common/common_param_traits.cc:145: std::vector<uint8_t> bytes; On 2017/05/19 03:38:50, Ryan Hamilton wrote: > On 2017/05/18 18:43:17, eroman wrote: > > For symmetry with GetSize() and Write(), can you change this to a > StackVector<> > > as well? > > Done. Though in order to do this, I needed to add a new IPAddress constructor > which takes a StackVector. But that seems like a good idea. As a result, I > replaced a number of calls to the existing std::vector constructor with this new > StackVector constructor. > > Does that sound good to you? PTAL. I am fine with adding a StackVector ctor if it is used elsewhere. But not sure I follow why it was a requirement for using StackVector in this particular function. i.e. I figure it would have been sufficient in this function to just change: *p = net::IPAddress(bytes); to: *p = net::IPAddress(bytes->data(), bytes->size()); https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc... net/base/ip_address.cc:153: for (size_t i = 0; i < size_; ++i) { Since you already include <algorithm> could do: return std::lexicographical_compare(begin(), end(), other.begin(), other.end()); https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc... net/base/ip_address.cc:162: bool IPAddress::IPAddressBytes::operator==( Or: return std::equal(begin(), end(), other.begin(), other.end()); https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc... net/base/ip_address.cc:299: base::StackVector<uint8_t, 16> bytes; optional: If you want to save a extra copy: CHECK_LE(num_zero_bytes, 16); IPAddress result; result.Resize(num_zero_bytes); std::fill(result.begin(), result.end(), 0); return result; https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.h#... net/base/ip_address.h:40: // of the underlying array or zero-initialze the bytes. typo: initialize https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.h#... net/base/ip_address.h:187: // vector, this method should be avoided unless absolutely necessary. optional: "Copies the bytes to a new vector. Generally callers should be using |bytes()| and the IPAddressBytes abstraction. This method is provided as a convenience for callsites that existed prior to the introduction of IPAddressBytes.". If you wanted to make this point more strongly, we can rename the function to: CopyBytesToVector(), then there is no doubt in callers minds that they are copying.
On 2017/05/19 16:04:04, yzshen1 wrote: > https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... > File net/dns/mojo_host_struct_traits.h (right): > > https://codereview.chromium.org/2881673002/diff/110001/net/dns/mojo_host_stru... > net/dns/mojo_host_struct_traits.h:54: return > std::vector<uint8_t>(obj.address().bytes().begin(), > On 2017/05/19 03:38:49, Ryan Hamilton wrote: > > On 2017/05/18 22:32:47, yzshen1 wrote: > > > On 2017/05/17 18:53:58, Randy Smith (Not in Mondays) wrote: > > > > On 2017/05/17 18:26:41, Ryan Hamilton wrote: > > > > > On 2017/05/15 22:15:23, eroman wrote: > > > > > > On 2017/05/13 13:20:47, Ryan Hamilton wrote: > > > > > > > On 2017/05/12 23:25:05, eroman wrote: > > > > > > > > .BytesAsVector() ? > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > Although per earlier comment, I think this should just be changed > to > > > > > > directly > > > > > > > > create the IPAddress without the vector temporary. > > > > > > > > > > > > > > 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? > > > > > > > > > > > > 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? > > > > > > > > > > rdsmith: do you know enough about mojo to help out here? > > > > > > > > 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. > > > > > > 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. > > > > 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? > > 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. *nod* Since in my use case, the data is *not* guaranteed to have a size of 16 it sounds like that'd not be a good choice here. > > I don't suppose mojo has an "array + length" type? > 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. *nod* I was hoping to avoid a heap allocation in this case so anything that ends up in an std::vector<> won't help. That being said, a heap allocation here is probably not the end of the world, so I'll just suck it up :)
Patchset #14 (id:250001) has been deleted
Ok, I think I've got this wrapped up. (Time to find non-net OWNERS :>) https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/2881673002/diff/210001/content/public/common/... content/public/common/common_param_traits.cc:145: std::vector<uint8_t> bytes; On 2017/05/19 17:37:54, eroman wrote: > On 2017/05/19 03:38:50, Ryan Hamilton wrote: > > On 2017/05/18 18:43:17, eroman wrote: > > > For symmetry with GetSize() and Write(), can you change this to a > > StackVector<> > > > as well? > > > > Done. Though in order to do this, I needed to add a new IPAddress constructor > > which takes a StackVector. But that seems like a good idea. As a result, I > > replaced a number of calls to the existing std::vector constructor with this > new > > StackVector constructor. > > > > Does that sound good to you? PTAL. > > I am fine with adding a StackVector ctor if it is used elsewhere. But not sure I > follow why it was a requirement for using StackVector in this particular > function. > > i.e. I figure it would have been sufficient in this function to just change: > > *p = net::IPAddress(bytes); > > to: > > *p = net::IPAddress(bytes->data(), bytes->size()); Oh, I see. Duh! Ok, I've removed the new StackVector constructor that I added, as well as the "legacy" std::vector constructor so as to discourage use of it. I've also converted most of the callers to use StackVector when possible. https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc... net/base/ip_address.cc:153: for (size_t i = 0; i < size_; ++i) { On 2017/05/19 17:37:54, eroman wrote: > Since you already include <algorithm> could do: > > return std::lexicographical_compare(begin(), end(), other.begin(), other.end()); BRILLIANT! I had no idea this existed, but it does explain references to "lexicographical_compare" I found in other documentation. Done. https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc... net/base/ip_address.cc:162: bool IPAddress::IPAddressBytes::operator==( On 2017/05/19 17:37:54, eroman wrote: > Or: > > return std::equal(begin(), end(), other.begin(), other.end()); Done. (though std::equal does not take 4 args, curiously) https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.cc... net/base/ip_address.cc:299: base::StackVector<uint8_t, 16> bytes; On 2017/05/19 17:37:54, eroman wrote: > optional: If you want to save a extra copy: > > CHECK_LE(num_zero_bytes, 16); > IPAddress result; > result.Resize(num_zero_bytes); > std::fill(result.begin(), result.end(), 0); > return result; There are a couple of problems with this. begin() and end are on IPAddressBytes not IPAddress. So that's easy. but then they return const refs not mutable refs. So how 'bout I just push_back(0u) on the underlying IPAddressBytes? https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.h#... net/base/ip_address.h:40: // of the underlying array or zero-initialze the bytes. On 2017/05/19 17:37:54, eroman wrote: > typo: initialize Done. https://codereview.chromium.org/2881673002/diff/230001/net/base/ip_address.h#... net/base/ip_address.h:187: // vector, this method should be avoided unless absolutely necessary. On 2017/05/19 17:37:54, eroman wrote: > optional: "Copies the bytes to a new vector. Generally callers should be using > |bytes()| and the IPAddressBytes abstraction. This method is provided as a > convenience for callsites that existed prior to the introduction of > IPAddressBytes.". Done. > If you wanted to make this point more strongly, we can rename the function to: > CopyBytesToVector(), then there is no doubt in callers minds that they are > copying. Done.
Description was changed from ========== Avoid heap allocations in IPAddress Use a helper class to represent the sequence of bytes in an IP address. A vector<uint8_t> would be simpler but incurs heap allocation, so IPAddressBytes uses a fixed size array. IPAddress construction and destruction is showing up in cronet profiles consuming roughly 5% of CPU. With this fix, it drops below .1%. ========== to ========== Avoid heap allocations in IPAddress Use a helper class to represent the sequence of bytes in an IP address. A vector<uint8_t> would be simpler but incurs heap allocation, so IPAddressBytes uses a fixed size array. IPAddress construction and destruction is showing up in cronet profiles consuming roughly 5% of CPU. With this fix, it drops below .1%. Also switch call sites which create IPAddress objects to avoid using std::vector and instead use base::StackVector. ==========
rch@chromium.org changed reviewers: + mbarbella@chromium.org, raymes@chromium.org, sergeyu@chromium.org
sergeyu: chrome/browser/media/webrtc/ remoting/host/ raymes: content/browser/renderer_host/pepper/ ppapi/shared_impl/private/ mbarbella: content/public/common/ tools/ipc_fuzzer/
https://codereview.chromium.org/2881673002/diff/290001/chrome/browser/media/w... File chrome/browser/media/webrtc/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/2881673002/diff/290001/chrome/browser/media/w... chrome/browser/media/webrtc/webrtc_text_log_handler.cc:81: base::StackVector<uint8_t, 16> bytes; How about without StackVector: net::IPAddressBytes stripped = address.bytes(); std::fill(stripped.begin() + 6, stripped.end(), 0); sensitive_address = net::IPAddress(stripped).ToString(); (The awkwardness of that last line suggests merging net::IPAddress / net::IPAddressBytes, or making IPAddress methods free-floating functions). https://codereview.chromium.org/2881673002/diff/290001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/290001/net/base/ip_address.cc... net/base/ip_address.cc:150: if (size_ < other.size_) optional: A shorter version: if (size_ == other.size()) return std::lexicographical_compare(...); return size_ < other.size(); https://codereview.chromium.org/2881673002/diff/290001/net/quic/platform/impl... File net/quic/platform/impl/quic_ip_address_impl.cc (right): https://codereview.chromium.org/2881673002/diff/290001/net/quic/platform/impl... net/quic/platform/impl/quic_ip_address_impl.cc:114: ip_address_ = IPAddress(ip->data(), ip->size()); Delete the three lines above, and change this one to: ip_address_ = IPAddress(data, length); (Doesn't seem like the memcpy is necessary). https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... File ppapi/shared_impl/private/net_address_private_impl.cc (right): https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... ppapi/shared_impl/private/net_address_private_impl.cc:500: &net_addr->address[address_size]); Technically this existing code was relying on undefined behavior (indexing beyond the end of the array), even if only to get the address. Can you change it to not use operator[]? Say: assign(net_addr->address, net_addr->address + address_size); https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... File ppapi/shared_impl/private/net_address_private_impl.h (right): https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... ppapi/shared_impl/private/net_address_private_impl.h:37: base::StackVector<uint8_t, 16>* address, Can this be changed to a IPAddressBytes? That would make it symmetric with the function above which does the reverse (and takes IPAddressBytes).
https://codereview.chromium.org/2881673002/diff/290001/chrome/browser/media/w... File chrome/browser/media/webrtc/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/2881673002/diff/290001/chrome/browser/media/w... chrome/browser/media/webrtc/webrtc_text_log_handler.cc:81: base::StackVector<uint8_t, 16> bytes; On 2017/05/19 22:00:07, eroman wrote: > How about without StackVector: > > net::IPAddressBytes stripped = address.bytes(); > std::fill(stripped.begin() + 6, stripped.end(), 0); > sensitive_address = net::IPAddress(stripped).ToString(); Done. (With minor modifications) > (The awkwardness of that last line suggests merging net::IPAddress / > net::IPAddressBytes, or making IPAddress methods free-floating functions). This seems like a fine idea, do you want that to happen now, or would a TODO suffice? https://codereview.chromium.org/2881673002/diff/290001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/290001/net/base/ip_address.cc... net/base/ip_address.cc:150: if (size_ < other.size_) On 2017/05/19 22:00:07, eroman wrote: > optional: A shorter version: > > if (size_ == other.size()) > return std::lexicographical_compare(...); > > return size_ < other.size(); Done. https://codereview.chromium.org/2881673002/diff/290001/net/quic/platform/impl... File net/quic/platform/impl/quic_ip_address_impl.cc (right): https://codereview.chromium.org/2881673002/diff/290001/net/quic/platform/impl... net/quic/platform/impl/quic_ip_address_impl.cc:114: ip_address_ = IPAddress(ip->data(), ip->size()); On 2017/05/19 22:00:07, eroman wrote: > Delete the three lines above, and change this one to: > > ip_address_ = IPAddress(data, length); > > (Doesn't seem like the memcpy is necessary). Done. (needs a reinterpret_cast though. Thanks, C++) https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... File ppapi/shared_impl/private/net_address_private_impl.cc (right): https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... ppapi/shared_impl/private/net_address_private_impl.cc:500: &net_addr->address[address_size]); On 2017/05/19 22:00:07, eroman wrote: > Technically this existing code was relying on undefined behavior (indexing > beyond the end of the array), even if only to get the address. > > Can you change it to not use operator[]? Say: > > assign(net_addr->address, net_addr->address + address_size); As per your previous comment, I switched this to take an IPAddressBytes instead of a StackVector, which does not have an assign method. Instead I push_back the bytes 1 at a time. (Of course, it's pushing back into an array so it's not going to reallocate or anything) https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... File ppapi/shared_impl/private/net_address_private_impl.h (right): https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... ppapi/shared_impl/private/net_address_private_impl.h:37: base::StackVector<uint8_t, 16>* address, On 2017/05/19 22:00:07, eroman wrote: > Can this be changed to a IPAddressBytes? That would make it symmetric with the > function above which does the reverse (and takes IPAddressBytes). Done.
remoting lgtm
https://codereview.chromium.org/2881673002/diff/290001/chrome/browser/media/w... File chrome/browser/media/webrtc/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/2881673002/diff/290001/chrome/browser/media/w... chrome/browser/media/webrtc/webrtc_text_log_handler.cc:81: base::StackVector<uint8_t, 16> bytes; On 2017/05/20 03:21:44, Ryan Hamilton wrote: > On 2017/05/19 22:00:07, eroman wrote: > > How about without StackVector: > > > > net::IPAddressBytes stripped = address.bytes(); > > std::fill(stripped.begin() + 6, stripped.end(), 0); > > sensitive_address = net::IPAddress(stripped).ToString(); > > Done. (With minor modifications) > > > (The awkwardness of that last line suggests merging net::IPAddress / > > net::IPAddressBytes, or making IPAddress methods free-floating functions). > > This seems like a fine idea, do you want that to happen now, or would a TODO > suffice? Let's worry about that later (Was just an observation). Thanks for asking though! https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... File ppapi/shared_impl/private/net_address_private_impl.cc (right): https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... ppapi/shared_impl/private/net_address_private_impl.cc:500: &net_addr->address[address_size]); On 2017/05/20 03:21:44, Ryan Hamilton wrote: > On 2017/05/19 22:00:07, eroman wrote: > > Technically this existing code was relying on undefined behavior (indexing > > beyond the end of the array), even if only to get the address. > > > > Can you change it to not use operator[]? Say: > > > > assign(net_addr->address, net_addr->address + address_size); > > As per your previous comment, I switched this to take an IPAddressBytes instead > of a StackVector, which does not have an assign method. Instead I push_back the > bytes 1 at a time. (Of course, it's pushing back into an array so it's not going > to reallocate or anything) This translation presumes that |address| is the empty address upon entry, which I don't think is guaranteed by the API. It would be prudent to call Resize(0) first to address this. Or better yet, add an Assign() method that takes ptr + size (and change the constructor to call this as a helper). https://codereview.chromium.org/2881673002/diff/240013/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/240013/net/base/ip_address.h#... net/base/ip_address.h:25: // Helper class to represent the sequence of bytes in an IP address. optional: I would suggest renaming this net::IPAddressBytes (i.e. not use an inner class). Inner classes are somewhat of a nuisance since you can't forward declare, and also the name IPAddress::IPAddressBytes() is kind of long.
traits and ipc_fuzzer lgtm
raymes, ping. Hopefully you can approve content/browser/renderer_host/pepper/ ppapi/shared_impl/private/
https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... File ppapi/shared_impl/private/net_address_private_impl.cc (right): https://codereview.chromium.org/2881673002/diff/290001/ppapi/shared_impl/priv... ppapi/shared_impl/private/net_address_private_impl.cc:500: &net_addr->address[address_size]); On 2017/05/22 18:06:44, eroman wrote: > On 2017/05/20 03:21:44, Ryan Hamilton wrote: > > On 2017/05/19 22:00:07, eroman wrote: > > > Technically this existing code was relying on undefined behavior (indexing > > > beyond the end of the array), even if only to get the address. > > > > > > Can you change it to not use operator[]? Say: > > > > > > assign(net_addr->address, net_addr->address + address_size); > > > > As per your previous comment, I switched this to take an IPAddressBytes > instead > > of a StackVector, which does not have an assign method. Instead I push_back > the > > bytes 1 at a time. (Of course, it's pushing back into an array so it's not > going > > to reallocate or anything) > > This translation presumes that |address| is the empty address upon entry, which > I don't think is guaranteed by the API. It would be prudent to call Resize(0) > first to address this. > > Or better yet, add an Assign() method that takes ptr + size (and change the > constructor to call this as a helper). Done. https://codereview.chromium.org/2881673002/diff/240013/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/240013/net/base/ip_address.h#... net/base/ip_address.h:25: // Helper class to represent the sequence of bytes in an IP address. On 2017/05/22 18:06:44, eroman wrote: > optional: I would suggest renaming this net::IPAddressBytes (i.e. not use an > inner class). Inner classes are somewhat of a nuisance since you can't forward > declare, and also the name IPAddress::IPAddressBytes() is kind of long. Done.
Sorry for the delay - didn't realize I was a reviewer. pepper lgtm
https://codereview.chromium.org/2881673002/diff/340001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/340001/net/base/ip_address.cc... net/base/ip_address.cc:148: std::copy_n(data, data_len, bytes_.data()); This is missing: size_ = data_len;
https://codereview.chromium.org/2881673002/diff/340001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/2881673002/diff/340001/net/base/ip_address.cc... net/base/ip_address.cc:148: std::copy_n(data, data_len, bytes_.data()); On 2017/05/22 22:46:53, eroman wrote: > This is missing: > size_ = data_len; *facepalm* Fixed and unit tests added.
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2881673002/diff/380001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/380001/net/base/ip_address.h#... net/base/ip_address.h:31: // Copys |data_len| elements from |data| into this object. "Copies" I believe.
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2881673002/diff/380001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/380001/net/base/ip_address.h#... net/base/ip_address.h:31: // Copys |data_len| elements from |data| into this object. On 2017/05/23 22:06:11, eroman wrote: > "Copies" I believe. Done.
https://codereview.chromium.org/2881673002/diff/420001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/420001/net/base/ip_address.h#... net/base/ip_address.h:31: // Copyies |data_len| elements from |data| into this object. Copies
https://codereview.chromium.org/2881673002/diff/420001/net/base/ip_address.h File net/base/ip_address.h (right): https://codereview.chromium.org/2881673002/diff/420001/net/base/ip_address.h#... net/base/ip_address.h:31: // Copyies |data_len| elements from |data| into this object. On 2017/05/24 20:07:28, eroman wrote: > Copies Kill me now. Done.
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, sergeyu@chromium.org, mbarbella@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2881673002/#ps460001 (title: "New constructor")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 460001, "attempt_start_ts": 1495737701354440,
"parent_rev": "ed233bea9377b9664fa0762b784e608c8635d0e6", "commit_rev":
"c6635f4e242cd2236f85a2b84cc42dfb945ad5d2"}
Message was sent while issue was closed.
Description was changed from ========== Avoid heap allocations in IPAddress Use a helper class to represent the sequence of bytes in an IP address. A vector<uint8_t> would be simpler but incurs heap allocation, so IPAddressBytes uses a fixed size array. IPAddress construction and destruction is showing up in cronet profiles consuming roughly 5% of CPU. With this fix, it drops below .1%. Also switch call sites which create IPAddress objects to avoid using std::vector and instead use base::StackVector. ========== to ========== Avoid heap allocations in IPAddress Use a helper class to represent the sequence of bytes in an IP address. A vector<uint8_t> would be simpler but incurs heap allocation, so IPAddressBytes uses a fixed size array. IPAddress construction and destruction is showing up in cronet profiles consuming roughly 5% of CPU. With this fix, it drops below .1%. Also switch call sites which create IPAddress objects to avoid using std::vector and instead use base::StackVector. Review-Url: https://codereview.chromium.org/2881673002 Cr-Commit-Position: refs/heads/master@{#474780} Committed: https://chromium.googlesource.com/chromium/src/+/c6635f4e242cd2236f85a2b84cc4... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001) as https://chromium.googlesource.com/chromium/src/+/c6635f4e242cd2236f85a2b84cc4... |
