Land Recent QUIC Changes.

Land Recent QUIC Changes.

Trying to merge as many of flow control changes as possible before the
branch (we have all changed until Fri 06/13 21:20 UTC).

Test tools for udp proxying.  This involves a few refactors of test
tools but nothing which affects the internal server.

minor refactors to allow quic udp proxy testing.

Merge internal change: 69157651
https://codereview.chromium.org/331963002/


Added FEC policy per stream, which is translated to an FEC protection
value on writes further on down.

Merge internal change: 69153464
https://codereview.chromium.org/338623002/


Pull out stream/session updates from OnConfigNegotiated. Preparation for
updating stream/session with different received windows.

QUIC refector: Pull out stream/session updates from OnConfigNegotiated

Merge internal change: 69106467
https://codereview.chromium.org/337723003/


Rather than passing initial_flow_control_window all the way down the
call stack, put it inside QuicConfig as intended: each member of
QuicConfig has a "value to send" field, so populate this at the top
level.

rtenneti: when porting to Chromium, you should add
config.SetInitialFlowControlWindowToSend(kInitialReceiveWindowSize)

in QuicStreamFactory::CreateSession, just above line 837: *session =
  new QuicClientSession(...)

Store initial flow control window for QUIC in QuicConfig. No behavior
change intended.

Added the following unit tests to EndToEndTest.cc
+ DoNotSetResumeWriteAlarmIfConnectionFlowControlBlocked
+ NegotiateMaxOpenStreams

Merge internal change: 69079363
https://codereview.chromium.org/333803007/


First version of a QUIC SendAlgorithmSimulator which is designed to
simulate BBR and TCP flows and changes.

Merge internal change: 69035927
https://codereview.chromium.org/330163003/

R=rch@chromium.org, wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277959

[ramant (doing other things), 2014-06-15 19:40:06]

Hi Ryan
  This CL is a merged CL of all the CL's you have LGTM'ed. PTAL.

thanks
raman

[wtc, 2014-06-16 19:35:57]

Patch set 1 LGTM.

Some of the changes I suggested should be made before committing this CL.

But you can ignore the "nit" changes related to function argument formatting.

Note: I feel this CL is too big. It was not easy to go through the whole CL even
though some of the changes (related to the removal of the initial congestion
window parameter) were quite straightforward.

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
File net/quic/congestion_control/send_algorithm_simulator.h (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
net/quic/congestion_control/send_algorithm_simulator.h:21: const QuicByteCount
kPacketSize = 1200;

Nit: in the net namespace, the constant name "kPacketSize" is too general. Can
this constant be moved into the SendAlgorithmSimulator class?

Update: kPacketSize is only used in send_algorithm_simulator.cc. So I would move
kPacketSize into send_algorithm_simulator.cc, inside an unnamed namespace.

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
net/quic/congestion_control/send_algorithm_simulator.h:43: virtual
~SendAlgorithmSimulator();

Nit: does the destructor need to be virtual? There is no other virtual method in
this class.

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
net/quic/congestion_control/send_algorithm_simulator.h:74: QuicByteCount
last_cwnd() { return last_cwnd_; }

Nit: declare these getter methods as const.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_client_session...
File net/quic/quic_client_session_base.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session_base.cc:13: config) {}

Nit: we can move this to the previous line now.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_protocol.h#new...
net/quic/quic_protocol.h:167: PROTECT_OPTIONAL  // Data in the stream does not
need FEC protection.

I suggest adding the "FEC_" prefix to these two enumerators because they can be
used without the FecPolicy:: prefix.

Also, I find MUST_FEC_PROTECT vs. MAY_FEC_PROTECT to be very similar to
PROTECT_ALWAYS vs. PROTECT_OPTIONAL. So I'm afraid that I would not be able to
know when to use FecProtection or FecPolicy.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.cc#new...
net/quic/quic_session.cc:490: it++;

Nit: ++it is more efficient.

You can also write this loop as a for loop. See lines 717-718.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.h
File net/quic/quic_session.h (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.h#newc...
net/quic/quic_session.h:57: const QuicConfig& config);

Nit: this may fit on the previous line now.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session_test.cc
File net/quic/quic_session_test.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session_test.c...
net/quic/quic_session_test.cc:122: DefaultQuicConfig()),

Nit: this should fit on the previous line.

https://codereview.chromium.org/330333006/diff/1/net/tools/quic/quic_client_s...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/tools/quic/quic_client_s...
net/tools/quic/quic_client_session.cc:23: config),

Nit: this should fit on the previous line.

[ramant (doing other things), 2014-06-16 21:13:53]

Many many thanks Wan-Teh for the thorough/awesome comments. Fixed all of them
and will port them back into our internal code base.

-raman

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
File net/quic/congestion_control/send_algorithm_simulator.h (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
net/quic/congestion_control/send_algorithm_simulator.h:21: const QuicByteCount
kPacketSize = 1200;
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: in the net namespace, the constant name "kPacketSize" is too general. Can
> this constant be moved into the SendAlgorithmSimulator class?
> 
> Update: kPacketSize is only used in send_algorithm_simulator.cc. So I would
move
> kPacketSize into send_algorithm_simulator.cc, inside an unnamed namespace.

Done.

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
net/quic/congestion_control/send_algorithm_simulator.h:43: virtual
~SendAlgorithmSimulator();
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: does the destructor need to be virtual? There is no other virtual method
in
> this class.

Done.

https://codereview.chromium.org/330333006/diff/1/net/quic/congestion_control/...
net/quic/congestion_control/send_algorithm_simulator.h:74: QuicByteCount
last_cwnd() { return last_cwnd_; }
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: declare these getter methods as const.

Done.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_client_session...
File net/quic/quic_client_session_base.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session_base.cc:13: config) {}
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: we can move this to the previous line now.

Done.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_protocol.h#new...
net/quic/quic_protocol.h:167: PROTECT_OPTIONAL  // Data in the stream does not
need FEC protection.
On 2014/06/16 19:35:56, wtc wrote:
> 
> I suggest adding the "FEC_" prefix to these two enumerators because they can
be
> used without the FecPolicy:: prefix.
> 
> Also, I find MUST_FEC_PROTECT vs. MAY_FEC_PROTECT to be very similar to
> PROTECT_ALWAYS vs. PROTECT_OPTIONAL. So I'm afraid that I would not be able to
> know when to use FecProtection or FecPolicy.

Added FEC_ prefix.

Done.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.cc#new...
net/quic/quic_session.cc:490: it++;
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: ++it is more efficient.
> 
> You can also write this loop as a for loop. See lines 717-718.

Done.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.h
File net/quic/quic_session.h (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session.h#newc...
net/quic/quic_session.h:57: const QuicConfig& config);
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: this may fit on the previous line now.

Done.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session_test.cc
File net/quic/quic_session_test.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session_test.c...
net/quic/quic_session_test.cc:122: DefaultQuicConfig()),
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: this should fit on the previous line.

Done.

https://codereview.chromium.org/330333006/diff/1/net/tools/quic/quic_client_s...
File net/tools/quic/quic_client_session.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/tools/quic/quic_client_s...
net/tools/quic/quic_client_session.cc:23: config),
On 2014/06/16 19:35:56, wtc wrote:
> 
> Nit: this should fit on the previous line.

Done.

[ramant (doing other things), 2014-06-16 21:14:01]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-06-16 21:15:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/330333006/20001

[Jana, 2014-06-16 21:28:12]

https://codereview.chromium.org/330333006/diff/20001/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/330333006/diff/20001/net/quic/quic_protocol.h...
net/quic/quic_protocol.h:164: // Indicates FEC policy
Per Wan-Teh's comment, I suggest a couple of things to help disambiguate the
meaning of these enums. (i) Expand this comment to say "Indicates FEC policy to
be used for this stream."  (ii) Change enum to be StreamFecPolicy and the values
to be FEC_PROTECT_STREAM_ALWAYS and  FEC_PROTECT_STREAM_OPTIONAL to clarify that
these policies are applied to a stream. Does that sound good? There are some
redundancies here, but I don't mind the extra clarification.

[wtc, 2014-06-16 22:05:53]

Patch set 2 LGTM.

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session_test.cc
File net/quic/quic_session_test.cc (right):

https://codereview.chromium.org/330333006/diff/1/net/quic/quic_session_test.c...
net/quic/quic_session_test.cc:122: DefaultQuicConfig()),

On 2014/06/16 21:13:53, ramant wrote:
> On 2014/06/16 19:35:56, wtc wrote:
> > 
> > Nit: this should fit on the previous line.
> 
> Done.

You didn't make this change. (It's fine to ignore this.)

https://codereview.chromium.org/330333006/diff/20001/net/quic/congestion_cont...
File net/quic/congestion_control/send_algorithm_simulator.h (right):

https://codereview.chromium.org/330333006/diff/20001/net/quic/congestion_cont...
net/quic/congestion_control/send_algorithm_simulator.h:67: const RttStats*
rtt_stats() { return rtt_stats_; }

Nit: this getter method should also be const.

https://codereview.chromium.org/330333006/diff/20001/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/330333006/diff/20001/net/quic/quic_protocol.h...
net/quic/quic_protocol.h:164: // Indicates FEC policy

On 2014/06/16 21:28:12, Jana wrote:
> Per Wan-Teh's comment, I suggest a couple of things to help disambiguate the
> meaning of these enums. (i) Expand this comment to say "Indicates FEC policy
to
> be used for this stream."  (ii) Change enum to be StreamFecPolicy and the
values
> to be FEC_PROTECT_STREAM_ALWAYS and  FEC_PROTECT_STREAM_OPTIONAL to clarify
that
> these policies are applied to a stream. Does that sound good?

Yes, that sounds good.

[ramant (doing other things), 2014-06-16 23:46:32]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2014-06-16 23:46:53]

https://codereview.chromium.org/330333006/diff/20001/net/quic/congestion_cont...
File net/quic/congestion_control/send_algorithm_simulator.h (right):

https://codereview.chromium.org/330333006/diff/20001/net/quic/congestion_cont...
net/quic/congestion_control/send_algorithm_simulator.h:67: const RttStats*
rtt_stats() { return rtt_stats_; }
On 2014/06/16 22:05:53, wtc wrote:
> 
> Nit: this getter method should also be const.

Done.

https://codereview.chromium.org/330333006/diff/20001/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/330333006/diff/20001/net/quic/quic_protocol.h...
net/quic/quic_protocol.h:164: // Indicates FEC policy
On 2014/06/16 21:28:12, Jana wrote:
> Per Wan-Teh's comment, I suggest a couple of things to help disambiguate the
> meaning of these enums. (i) Expand this comment to say "Indicates FEC policy
to
> be used for this stream."  (ii) Change enum to be StreamFecPolicy and the
values
> to be FEC_PROTECT_STREAM_ALWAYS and  FEC_PROTECT_STREAM_OPTIONAL to clarify
that
> these policies are applied to a stream. Does that sound good? There are some
> redundancies here, but I don't mind the extra clarification.

Will make this change in the next CL.

[commit-bot: I haz the power, 2014-06-16 23:49:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/330333006/40001

[wtc, 2014-06-17 00:07:37]

Patch set 3 LGTM.

https://codereview.chromium.org/330333006/diff/40001/net/quic/congestion_cont...
File net/quic/congestion_control/send_algorithm_simulator.cc (right):

https://codereview.chromium.org/330333006/diff/40001/net/quic/congestion_cont...
net/quic/congestion_control/send_algorithm_simulator.cc:19: const
net::QuicByteCount kPacketSize = 1200;

Another solution is to nest this inside the net namespace. You can fix this in a
follow-up CL.

[ramant (doing other things), 2014-06-17 02:32:31]

https://codereview.chromium.org/330333006/diff/40001/net/quic/congestion_cont...
File net/quic/congestion_control/send_algorithm_simulator.cc (right):

https://codereview.chromium.org/330333006/diff/40001/net/quic/congestion_cont...
net/quic/congestion_control/send_algorithm_simulator.cc:19: const
net::QuicByteCount kPacketSize = 1200;
On 2014/06/17 00:07:37, wtc wrote:
> 
> Another solution is to nest this inside the net namespace. You can fix this in
a
> follow-up CL.

Done.

[ramant (doing other things), 2014-06-17 02:32:38]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-06-17 02:34:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/330333006/60001

[commit-bot: I haz the power, 2014-06-17 08:41:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 08:41:32]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[wtc, 2014-06-17 16:29:41]

Patch set 4 LGTM.

[ramant (doing other things), 2014-06-17 19:16:15]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-06-17 19:18:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/330333006/100001

[ramant (doing other things), 2014-06-17 21:22:36]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-06-17 21:23:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/330333006/120001

[commit-bot: I haz the power, 2014-06-18 07:02:04]

Change committed as 277959