Consistently suggest ephemeral port for QUIC client

net/quic/port_suggester.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/quic/port_suggester.h"
+
+#include "base/metrics/histogram.h"
+#include "net/base/host_port_pair.h"
+
+namespace net {
+
+PortSuggester::PortSuggester(const HostPortPair& server,
+                             uint64 per_profile_randomness)
+    : count_(0) {
+  base::SHA1HashBytes(
+      reinterpret_cast<const unsigned char*>(server.host().data()),
+      server.host().length(), seed_.hash_bytes);
+
+  DCHECK_LE(sizeof(seed_.first_uint64), sizeof(seed_.hash_bytes));

[Ryan Hamilton, 2013/12/07 01:46:14]

Can you use COMPILE_ASSERT here?

[jar (doing other things), 2013/12/07 03:12:18]

Done.

[Ryan Hamilton, 2013/12/07 23:21:54]

It looks like this check has been removed.  I'm totally fine with that, but if
you wished, you could do something like:

  COMPILE_ASSERT(sizeof(seed_) < base::kSHA1Length);

[jar (doing other things), 2013/12/08 01:38:36]

Done.

+  seed_.first_uint64 ^= per_profile_randomness ^ server.port();

[Ryan Hamilton, 2013/12/07 01:46:14]

Since you've already gone through the trouble of using a sha1 hash, I'm
surprised to see the use of XOR here.  Did you consider including all this data
as initial input to the sha1 hash?

[jar (doing other things), 2013/12/07 03:12:18]

The only item that should not be (close to) uniformly random is server.port(). 
As a result, I think I get no reduction in the randomness of the result by just
XORing in that one item.

[Ryan Hamilton, 2013/12/07 04:52:22]

Ah, I see.  Makes sense.

+}
+
+PortSuggester::~PortSuggester() {
+  UMA_HISTOGRAM_COUNTS("Net.Quic.EphemeralPortsSuggested", count_);
+}
+
+int PortSuggester::SuggestPort(int min, int max) {
+  // Sometimes our suggestion can't be used, so we ensure that if a additional

[wtc, 2013/12/07 01:03:38]

Nit: remove "a"

[jar (doing other things), 2013/12/07 03:12:18]

Done.

+  // calls are made, that each call (probably) provides a new suggestion.

[wtc, 2013/12/07 01:03:38]

Nit: I think "that" can be removed

[jar (doing other things), 2013/12/07 03:12:18]

Done.

+  if (++count_ > 1) {
+    // Evolve the seed.
+    base::SHA1HashBytes(seed_.hash_bytes, sizeof(seed_.hash_bytes),
+                        seed_.hash_bytes);
+  }
+  int rand = std::abs(seed_.first_int);

[Ryan Hamilton, 2013/12/07 01:46:14]

I notice that you're using first_int here, and first_uint64 is never use after
the constructor?  If seed_ were simply a uint64, I think you could get rid of
this abs step, and just operate on seed_ directly:

int range = max - min + 1;
return (seed_ % range) + min;

Since seed is unsigned, seed_ % range will be unsigned, and in the right range. 

[jar (doing other things), 2013/12/07 03:12:18]

Seed_ is currently large enough to hold all the return hash bits... which makes
the calls a bit cleaner... and avoids doing any stack  allocation (declaration)
and memcopy (to truncate).

My expectation was that we didn't need to do any 64 bit math, as the port range
was generally 16 bits, and we pulled out 32 bits to do the modulo operation.

My guess is that Android will actually want to further simplify this calculation
(even with 32 bits, let alone 64), as the ARM seems averse to divisions... so I
figured it was nicer to leave this simple.

If you really want it, I can change it in any direction... but I doubt it will
be simpler.

[Ryan Hamilton, 2013/12/07 04:52:22]

Understood.  However, in my opinion the extra computational overhead does not
seem to merit the decrease in readability.
If android and arm are averse to 64 bit math, then they're going to have a
conniption  fit with QUIC :>  We do all sorts for 64 bit math on every packet. 
Recall that we do a bunch of 64 bit arithmetic to figure out the correct high
bits of the sequence number for example.  We do this on every *packet*.  The
port suggestion happens typically once per connection.
 
Yes, I think it is clearer.  It will remove one of the fields of the union (I
think we can get rid of the other, too, see other comments :>) and it will
remove the call to abs.  This is more readable to me.

+  DCHECK_LE(min, max);
+  DCHECK_GT(min, 0);
+  int range = max - min + 1;
+  // Ports (and hence the extent of the |range|) are generally under 2^16, while
+  // ints are bigger than 2^31, so the tiny non-uniformity in the pseudo-random
+  // distribution is not significant.
+  return (rand % range) + min;
+}
+
+}  // namespace net