Landing Recent QUIC changes until 11:17 PM, Oct 12, 2017

deprecate FLAGS_quic_reloadable_flag_quic_disable_packets_based_cc Merge internal change: 17201012 https://chromium-review.googlesource.com/c/chromium/src/+/728343 In QUIC, print range of packet numbers in ack frame when there are too many (> 128). No functional change expected. Modify quic_ack_frame::operator<< to print out a range rather than explict packet numbers when there would be too many numbers to print. Merge internal change: 171947147 https://chromium-review.googlesource.com/c/chromium/src/+/728394 Use go/portableproto for QUIC protos. Merge internal change: 171878936 https://chromium-review.googlesource.com/c/chromium/src/+/728391 Add a new BBR connection option to use a slower STARTUP once loss occurs. Protected by FLAGS_quic_reloadable_flag_quic_bbr_slower_startup Merge internal change: 171744905 https://chromium-review.googlesource.com/c/chromium/src/+/728390 Deprecate FLAGS_quic_reloadable_flag_quic_bbr_exit_startup_on_loss. n/a (Flag deprecation) Merge internal change: 171680259 https://chromium-review.googlesource.com/c/chromium/src/+/728388 In QUIC, remove ack listener list from SerializedPacket and TransmissionInfo. Also remove ack listener from the write path. No functional change expected. Merge internal change: 171537663 https://chromium-review.googlesource.com/c/chromium/src/+/728385 Allows self address change for QUIC proxied sessions. Protected by FLAGS_quic_reloadable_flag_quic_allow_address_change_for_udp_proxy. Merge internal change: 171526707 https://chromium-review.googlesource.com/c/chromium/src/+/728303 R=rch@chromium.org Bug: Change-Id: I63f023d75ebc07dccb0956f21fbcaa018e46f549 Reviewed-on: https://chromium-review.googlesource.com/728701 Commit-Queue: Ian Swett <ianswett@chromium.org> Reviewed-by: Ryan Hamilton <rch@chromium.org> Cr-Commit-Position: refs/heads/master@{#510467}
Denger-Network · Oct 20, 2017 · 774f2bb · 774f2bb
1 parent 8f9c714
commit 774f2bb
Show file tree

Hide file tree

Showing 52 changed files with 478 additions and 2,202 deletions.
diff --git a/net/BUILD.gn b/net/BUILD.gn
@@ -1169,8 +1169,6 @@ component("net") {
       "quic/core/congestion_control/tcp_cubic_sender_base.h",
       "quic/core/congestion_control/tcp_cubic_sender_bytes.cc",
       "quic/core/congestion_control/tcp_cubic_sender_bytes.h",
-      "quic/core/congestion_control/tcp_cubic_sender_packets.cc",
-      "quic/core/congestion_control/tcp_cubic_sender_packets.h",
       "quic/core/congestion_control/windowed_filter.h",
       "quic/core/crypto/aead_base_decrypter.cc",
       "quic/core/crypto/aead_base_decrypter.h",
@@ -4989,8 +4987,6 @@ test("net_unittests") {
     "quic/core/congestion_control/prr_sender_test.cc",
     "quic/core/congestion_control/rtt_stats_test.cc",
     "quic/core/congestion_control/send_algorithm_test.cc",
-    "quic/core/congestion_control/tcp_cubic_sender_bytes_test.cc",
-    "quic/core/congestion_control/tcp_cubic_sender_packets_test.cc",
     "quic/core/congestion_control/windowed_filter_test.cc",
     "quic/core/crypto/aes_128_gcm_12_decrypter_test.cc",
     "quic/core/crypto/aes_128_gcm_12_encrypter_test.cc",

diff --git a/net/quic/chromium/quic_chromium_client_session_test.cc b/net/quic/chromium/quic_chromium_client_session_test.cc
@@ -1153,8 +1153,7 @@ TEST_P(QuicChromiumClientSessionTest, MigrateToSocket) {
       QuicIOVector(iov, arraysize(iov), 4), 0, 4);
   QuicStreamPeer::SetStreamBytesWritten(4, stream);
   session_->WritevData(stream, stream->id(),
-                       QuicIOVector(iov, arraysize(iov), 4), 0, NO_FIN,
-                       nullptr);
+                       QuicIOVector(iov, arraysize(iov), 4), 0, NO_FIN);
 
   EXPECT_TRUE(socket_data.AllReadDataConsumed());
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());

diff --git a/net/quic/chromium/quic_chromium_client_stream_test.cc b/net/quic/chromium/quic_chromium_client_stream_test.cc
@@ -59,14 +59,12 @@ class MockQuicClientSessionBase : public QuicSpdyClientSessionBase {
                     ConnectionCloseSource source));
   MOCK_METHOD1(CreateIncomingDynamicStream, QuicSpdyStream*(QuicStreamId id));
   MOCK_METHOD0(CreateOutgoingDynamicStream, QuicChromiumClientStream*());
-  MOCK_METHOD6(
-      WritevData,
-      QuicConsumedData(QuicStream* stream,
-                       QuicStreamId id,
-                       QuicIOVector data,
-                       QuicStreamOffset offset,
-                       StreamSendingState fin,
-                       QuicReferenceCountedPointer<QuicAckListenerInterface>));
+  MOCK_METHOD5(WritevData,
+               QuicConsumedData(QuicStream* stream,
+                                QuicStreamId id,
+                                QuicIOVector data,
+                                QuicStreamOffset offset,
+                                StreamSendingState fin));
   MOCK_METHOD3(SendRstStream,
                void(QuicStreamId stream_id,
                     QuicRstStreamErrorCode error,
@@ -146,7 +144,7 @@ MockQuicClientSessionBase::MockQuicClientSessionBase(
                                 DefaultQuicConfig()) {
   crypto_stream_.reset(new MockQuicCryptoStream(this));
   Initialize();
-  ON_CALL(*this, WritevData(_, _, _, _, _, _))
+  ON_CALL(*this, WritevData(_, _, _, _, _))
       .WillByDefault(testing::Return(QuicConsumedData(0, false)));
 }
 
@@ -281,7 +279,7 @@ TEST_P(QuicChromiumClientStreamTest, Handle) {
   const size_t kDataLen = arraysize(kData1);
 
   // All data written.
-  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _, _))
+  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _))
       .WillOnce(Return(QuicConsumedData(kDataLen, true)));
   TestCompletionCallback callback;
   EXPECT_EQ(OK, handle_->WriteStreamData(QuicStringPiece(kData1, kDataLen),
@@ -567,7 +565,7 @@ TEST_P(QuicChromiumClientStreamTest, WriteStreamData) {
   const size_t kDataLen = arraysize(kData1);
 
   // All data written.
-  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _, _))
+  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _))
       .WillOnce(Return(QuicConsumedData(kDataLen, true)));
   TestCompletionCallback callback;
   EXPECT_EQ(OK, handle_->WriteStreamData(QuicStringPiece(kData1, kDataLen),
@@ -579,7 +577,7 @@ TEST_P(QuicChromiumClientStreamTest, WriteStreamDataAsync) {
   const size_t kDataLen = arraysize(kData1);
 
   // No data written.
-  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _, _))
+  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _))
       .WillOnce(Return(QuicConsumedData(0, false)));
   TestCompletionCallback callback;
   EXPECT_EQ(ERR_IO_PENDING,
@@ -588,7 +586,7 @@ TEST_P(QuicChromiumClientStreamTest, WriteStreamDataAsync) {
   ASSERT_FALSE(callback.have_result());
 
   // All data written.
-  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _, _))
+  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _))
       .WillOnce(Return(QuicConsumedData(kDataLen, true)));
   stream_->OnCanWrite();
   ASSERT_TRUE(callback.have_result());
@@ -601,7 +599,7 @@ TEST_P(QuicChromiumClientStreamTest, WritevStreamData) {
       new StringIOBuffer("Just a small payload"));
 
   // All data written.
-  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _, _))
+  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _))
       .WillOnce(Return(QuicConsumedData(buf1->size(), false)))
       .WillOnce(Return(QuicConsumedData(buf2->size(), true)));
   TestCompletionCallback callback;
@@ -616,7 +614,7 @@ TEST_P(QuicChromiumClientStreamTest, WritevStreamDataAsync) {
       new StringIOBuffer("Just a small payload"));
 
   // Only a part of the data is written.
-  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _, _))
+  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _))
       // First piece of data is written.
       .WillOnce(Return(QuicConsumedData(buf1->size(), false)))
       // Second piece of data is queued.
@@ -629,7 +627,7 @@ TEST_P(QuicChromiumClientStreamTest, WritevStreamDataAsync) {
   ASSERT_FALSE(callback.have_result());
 
   // The second piece of data is written.
-  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _, _))
+  EXPECT_CALL(session_, WritevData(stream_, stream_->id(), _, _, _))
       .WillOnce(Return(QuicConsumedData(buf2->size(), true)));
   stream_->OnCanWrite();
   ASSERT_TRUE(callback.have_result());

diff --git a/net/quic/core/congestion_control/bbr_sender.cc b/net/quic/core/congestion_control/bbr_sender.cc
@@ -26,6 +26,10 @@ const QuicByteCount kMinimumCongestionWindow = 4 * kMaxSegmentSize;
 
 // The gain used for the slow start, equal to 2/ln(2).
 const float kHighGain = 2.885f;
+// The gain used in STARTUP after loss has been detected.
+// 1.5 is enough to allow for 25% exogenous loss and still observe a 25% growth
+// in measured bandwidth.
+const float kStartupAfterLossGain = 1.5f;
 // The gain used to drain the queue after the slow start.
 const float kDrainGain = 1.f / kHighGain;
 // The cycle of gains used during the PROBE_BW stage.
@@ -40,7 +44,6 @@ const QuicRoundTripCount kBandwidthWindowSize = kGainCycleLength + 2;
 const QuicTime::Delta kMinRttExpiry = QuicTime::Delta::FromSeconds(10);
 // The minimum time the connection can spend in PROBE_RTT mode.
 const QuicTime::Delta kProbeRttTime = QuicTime::Delta::FromMilliseconds(200);
-
 // If the bandwidth does not increase by the factor of |kStartupGrowthTarget|
 // within |kRoundTripsWithoutGrowthBeforeExitingStartup| rounds, the connection
 // will exit the STARTUP mode.
@@ -113,7 +116,8 @@ BbrSender::BbrSender(const RttStats* rtt_stats,
       recovery_state_(NOT_IN_RECOVERY),
       end_recovery_at_(0),
       recovery_window_(max_congestion_window_),
-      rate_based_recovery_(false) {
+      rate_based_recovery_(false),
+      slower_startup_(false) {
   EnterStartupMode();
 }
 
@@ -184,9 +188,7 @@ bool BbrSender::IsProbingForMoreBandwidth() const {
 
 void BbrSender::SetFromConfig(const QuicConfig& config,
                               Perspective perspective) {
-  if (FLAGS_quic_reloadable_flag_quic_bbr_exit_startup_on_loss &&
-      config.HasClientRequestedIndependentOption(kLRTT, perspective)) {
-    QUIC_FLAG_COUNT(quic_reloadable_flag_quic_bbr_exit_startup_on_loss);
+  if (config.HasClientRequestedIndependentOption(kLRTT, perspective)) {
     exit_startup_on_loss_ = true;
   }
   if (config.HasClientRequestedIndependentOption(k1RTT, perspective)) {
@@ -205,6 +207,11 @@ void BbrSender::SetFromConfig(const QuicConfig& config,
   if (config.HasClientRequestedIndependentOption(kBBR2, perspective)) {
     max_aggregation_bytes_multiplier_ = 2;
   }
+  if (FLAGS_quic_reloadable_flag_quic_bbr_slower_startup &&
+      config.HasClientRequestedIndependentOption(kBBRS, perspective)) {
+    QUIC_FLAG_COUNT(quic_reloadable_flag_quic_bbr_slower_startup);
+    slower_startup_ = true;
+  }
 }
 
 void BbrSender::AdjustNetworkParameters(QuicBandwidth bandwidth,
@@ -574,6 +581,12 @@ void BbrSender::CalculatePacingRate() {
         initial_congestion_window_, rtt_stats_->min_rtt());
     return;
   }
+  // Slow the pacing rate in STARTUP once loss has ever been detected.
+  const bool has_ever_detected_loss = end_recovery_at_ > 0;
+  if (slower_startup_ && has_ever_detected_loss) {
+    pacing_rate_ = kStartupAfterLossGain * BandwidthEstimate();
+    return;
+  }
 
   // Do not decrease the pacing rate during the startup.
   pacing_rate_ = std::max(pacing_rate_, target_rate);

diff --git a/net/quic/core/congestion_control/bbr_sender.h b/net/quic/core/congestion_control/bbr_sender.h
@@ -311,13 +311,16 @@ class QUIC_EXPORT_PRIVATE BbrSender : public SendAlgorithmInterface {
   // Current state of recovery.
   RecoveryState recovery_state_;
   // Receiving acknowledgement of a packet after |end_recovery_at_| will cause
-  // BBR to exit the recovery mode.
+  // BBR to exit the recovery mode.  A value above zero indicates at least one
+  // loss has been detected, so it must not be set back to zero.
   QuicPacketNumber end_recovery_at_;
   // A window used to limit the number of bytes in flight during loss recovery.
   QuicByteCount recovery_window_;
 
   // When true, recovery is rate based rather than congestion window based.
   bool rate_based_recovery_;
+  // When true, pace at 1.5x and disable packet conservation in STARTUP.
+  bool slower_startup_;
 
   DISALLOW_COPY_AND_ASSIGN(BbrSender);
 };

diff --git a/net/quic/core/congestion_control/bbr_sender_test.cc b/net/quic/core/congestion_control/bbr_sender_test.cc
@@ -731,7 +731,6 @@ TEST_F(BbrSenderTest, SimpleTransfer2RTTStartup) {
 
 // Test exiting STARTUP earlier upon loss due to the LRTT connection option.
 TEST_F(BbrSenderTest, SimpleTransferLRTTStartup) {
-  FLAGS_quic_reloadable_flag_quic_bbr_exit_startup_on_loss = true;
   CreateDefaultSetup();
 
   SetConnectionOption(kLRTT);
@@ -760,7 +759,6 @@ TEST_F(BbrSenderTest, SimpleTransferLRTTStartup) {
 
 // Test exiting STARTUP earlier upon loss due to the LRTT connection option.
 TEST_F(BbrSenderTest, SimpleTransferLRTTStartupSmallBuffer) {
-  FLAGS_quic_reloadable_flag_quic_bbr_exit_startup_on_loss = true;
   CreateSmallBufferSetup();
 
   SetConnectionOption(kLRTT);
@@ -787,6 +785,43 @@ TEST_F(BbrSenderTest, SimpleTransferLRTTStartupSmallBuffer) {
   EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
 }
 
+// Test slower pacing after loss in STARTUP due to the BBRS connection option.
+TEST_F(BbrSenderTest, SimpleTransferSlowerStartup) {
+  // Adding TSO CWND causes packet loss before exiting startup.
+  FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
+  FLAGS_quic_reloadable_flag_quic_bbr_slower_startup = true;
+  CreateSmallBufferSetup();
+
+  SetConnectionOption(kBBRS);
+  EXPECT_EQ(3u, sender_->num_startup_rtts());
+
+  // Run until the full bandwidth is reached and check how many rounds it was.
+  bbr_sender_.AddBytesToTransfer(12 * 1024 * 1024);
+  QuicRoundTripCount max_bw_round = 0;
+  QuicBandwidth max_bw(QuicBandwidth::Zero());
+  bool simulator_result = simulator_.RunUntilOrTimeout(
+      [this, &max_bw, &max_bw_round]() {
+        if (max_bw < sender_->ExportDebugState().max_bandwidth) {
+          max_bw = sender_->ExportDebugState().max_bandwidth;
+          max_bw_round = sender_->ExportDebugState().round_trip_count;
+        }
+        // Expect the pacing rate in STARTUP to decrease once packet loss
+        // is observed, but the CWND does not.
+        if (bbr_sender_.connection()->GetStats().packets_lost > 0 &&
+            !sender_->ExportDebugState().is_at_full_bandwidth) {
+          EXPECT_EQ(1.5f * max_bw, sender_->PacingRate(0));
+        }
+        return sender_->ExportDebugState().is_at_full_bandwidth;
+      },
+      QuicTime::Delta::FromSeconds(5));
+  ASSERT_TRUE(simulator_result);
+  EXPECT_EQ(BbrSender::DRAIN, sender_->ExportDebugState().mode);
+  EXPECT_GE(3u, sender_->ExportDebugState().round_trip_count - max_bw_round);
+  EXPECT_EQ(3u, sender_->ExportDebugState().rounds_without_bandwidth_gain);
+  EXPECT_NE(0u, bbr_sender_.connection()->GetStats().packets_lost);
+  EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
+}
+
 // Test that two BBR flows started slightly apart from each other terminate.
 TEST_F(BbrSenderTest, SimpleCompetition) {
   const QuicByteCount transfer_size = 10 * 1024 * 1024;

diff --git a/net/quic/core/congestion_control/send_algorithm_interface.cc b/net/quic/core/congestion_control/send_algorithm_interface.cc
@@ -6,7 +6,6 @@
 
 #include "net/quic/core/congestion_control/bbr_sender.h"
 #include "net/quic/core/congestion_control/tcp_cubic_sender_bytes.h"
-#include "net/quic/core/congestion_control/tcp_cubic_sender_packets.h"
 #include "net/quic/core/quic_packets.h"
 #include "net/quic/platform/api/quic_bug_tracker.h"
 #include "net/quic/platform/api/quic_flag_utils.h"
@@ -39,26 +38,10 @@ SendAlgorithmInterface* SendAlgorithmInterface::Create(
                                max_congestion_window);
       }
     // Fall back to CUBIC if PCC is disabled.
-    case kCubic:
-      if (!FLAGS_quic_reloadable_flag_quic_disable_packets_based_cc) {
-        return new TcpCubicSenderPackets(
-            clock, rtt_stats, false /* don't use Reno */,
-            initial_congestion_window, max_congestion_window, stats);
-      }
-      QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_disable_packets_based_cc, 1,
-                        2);
     case kCubicBytes:
       return new TcpCubicSenderBytes(
           clock, rtt_stats, false /* don't use Reno */,
           initial_congestion_window, max_congestion_window, stats);
-    case kReno:
-      if (!FLAGS_quic_reloadable_flag_quic_disable_packets_based_cc) {
-        return new TcpCubicSenderPackets(clock, rtt_stats, true /* use Reno */,
-                                         initial_congestion_window,
-                                         max_congestion_window, stats);
-      }
-      QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_disable_packets_based_cc, 2,
-                        2);
     case kRenoBytes:
       return new TcpCubicSenderBytes(clock, rtt_stats, true /* use Reno */,
                                      initial_congestion_window,

diff --git a/net/quic/core/congestion_control/send_algorithm_test.cc b/net/quic/core/congestion_control/send_algorithm_test.cc
@@ -105,12 +105,8 @@ const QuicTime::Delta kTestLinkSmallRTTDelay =
 
 const char* CongestionControlTypeToString(CongestionControlType cc_type) {
   switch (cc_type) {
-    case kCubic:
-      return "CUBIC_PACKETS";
     case kCubicBytes:
       return "CUBIC_BYTES";
-    case kReno:
-      return "RENO_PACKETS";
     case kRenoBytes:
       return "RENO_BYTES";
     case kBBR:
@@ -166,11 +162,10 @@ string TestParamToString(const testing::TestParamInfo<TestParams>& params) {
 std::vector<TestParams> GetTestParams() {
   std::vector<TestParams> params;
   for (const CongestionControlType congestion_control_type :
-       {kBBR, kCubic, kCubicBytes, kReno, kRenoBytes, kPCC}) {
+       {kBBR, kCubicBytes, kRenoBytes, kPCC}) {
     params.push_back(
         TestParams(congestion_control_type, false, false, false, false));
-    if (congestion_control_type != kCubic &&
-        congestion_control_type != kCubicBytes) {
+    if (congestion_control_type != kCubicBytes) {
       continue;
     }
     params.push_back(

diff --git a/net/quic/core/congestion_control/tcp_cubic_sender_base.h b/net/quic/core/congestion_control/tcp_cubic_sender_base.h
@@ -11,7 +11,6 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
-#include "net/quic/core/congestion_control/cubic.h"
 #include "net/quic/core/congestion_control/hybrid_slow_start.h"
 #include "net/quic/core/congestion_control/prr_sender.h"
 #include "net/quic/core/congestion_control/send_algorithm_interface.h"