Landing Recent QUIC changes until 1:52 PM, Jul 28, 2017 UTC-4

Update feature flags

https://chromium-review.googlesource.com/c/596150/

Deprecate quic_reloadable_flag_quic_faster_bandwidth_sampler.

Merge internal change: 163489610

https://chromium-review.googlesource.com/c/595337/

Remove unused methods

Merge internal change: 163472196

https://chromium-review.googlesource.com/c/595336/

Adding two new QUIC BBR ack aggregation modes.

The modes are requested with the BBR1 and BBR2 connection options and
protected by quic_reloadable_flag_quic_bbr_ack_aggregation4, which
replaces quic_reloadable_flag_quic_bbr_ack_aggregation3.

Merge internal change: 163344044

https://chromium-review.googlesource.com/c/595335/

Deprecate quic_reloadable_flag_quic_bbr_ack_aggregation2 in the false state.

Merge internal change: 163252118

https://chromium-review.googlesource.com/c/595333/

Change QUIC ack type byte to match IETF 101NLLMM format

As specified by
https://github.com/quicwg/base-drafts/commit/fac7f9d91f781602d8425ccd1fa6aa521f78243a.

Merge internal change: 163211775

https://chromium-review.googlesource.com/c/594855/

R=rch@chromium.org

Bug: 
Change-Id: I7c7e1af913f98c8c8dfd4d98551ae320c0febf5d
Reviewed-on: https://chromium-review.googlesource.com/596227
Commit-Queue: Victor Vasiliev <vasilvv@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491158}

net/quic/core/congestion_control/bandwidth_sampler.cc

@@ -20,10 +20,7 @@ BandwidthSampler::BandwidthSampler()
       last_sent_packet_(0),
       is_app_limited_(false),
       end_of_app_limited_phase_(0),
-      connection_state_map_(),
-      connection_state_map_new_(),
-      use_new_connection_state_map_(
-          FLAGS_quic_reloadable_flag_quic_faster_bandwidth_sampler) {}
+      connection_state_map_() {}
 
 BandwidthSampler::~BandwidthSampler() {}
 
@@ -56,61 +53,33 @@ void BandwidthSampler::OnPacketSent(
     last_acked_packet_sent_time_ = sent_time;
   }
 
-  if (use_new_connection_state_map_) {
-    if (!connection_state_map_new_.IsEmpty() &&
-        packet_number >
-            connection_state_map_new_.last_packet() + kMaxTrackedPackets) {
-      QUIC_BUG << "BandwidthSampler in-flight packet map has exceeded maximum "
-                  "number "
-                  "of tracked packets.";
-    }
-
-    bool success = connection_state_map_new_.Emplace(packet_number, sent_time,
-                                                     bytes, *this);
-    QUIC_BUG_IF(!success) << "BandwidthSampler failed to insert the packet "
-                             "into the map, most likely because it's already "
-                             "in it.";
-    return;
+  if (!connection_state_map_.IsEmpty() &&
+      packet_number >
+          connection_state_map_.last_packet() + kMaxTrackedPackets) {
+    QUIC_BUG << "BandwidthSampler in-flight packet map has exceeded maximum "
+                "number "
+                "of tracked packets.";
   }
 
-  DCHECK(connection_state_map_.find(packet_number) ==
-         connection_state_map_.end());
-  connection_state_map_.emplace(
-      packet_number, ConnectionStateOnSentPacket(sent_time, bytes, *this));
-
-  QUIC_BUG_IF(connection_state_map_.size() > kMaxTrackedPackets)
-      << "BandwidthSampler in-flight packet map has exceeded maximum number "
-         "of tracked packets.";
+  bool success =
+      connection_state_map_.Emplace(packet_number, sent_time, bytes, *this);
+  QUIC_BUG_IF(!success) << "BandwidthSampler failed to insert the packet "
+                           "into the map, most likely because it's already "
+                           "in it.";
 }
 
 BandwidthSample BandwidthSampler::OnPacketAcknowledged(
     QuicTime ack_time,
     QuicPacketNumber packet_number) {
-  if (use_new_connection_state_map_) {
-    ConnectionStateOnSentPacket* sent_packet_pointer =
-        connection_state_map_new_.GetEntry(packet_number);
-    if (sent_packet_pointer == nullptr) {
-      // See the TODO below.
-      return BandwidthSample();
-    }
-    BandwidthSample sample = OnPacketAcknowledgedInner(ack_time, packet_number,
-                                                       *sent_packet_pointer);
-    connection_state_map_new_.Remove(packet_number);
-    QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_faster_bandwidth_sampler, 1, 2);
-    return sample;
-  }
-
-  auto it = connection_state_map_.find(packet_number);
-  if (it == connection_state_map_.end()) {
-    // TODO(vasilvv): currently, this can happen because the congestion
-    // controller can be created while some of the handshake packets are still
-    // in flight.  Once the sampler is fully integrated with unacked packet map,
-    // this should be a QUIC_BUG equivalent.
+  ConnectionStateOnSentPacket* sent_packet_pointer =
+      connection_state_map_.GetEntry(packet_number);
+  if (sent_packet_pointer == nullptr) {
+    // See the TODO below.
     return BandwidthSample();
   }
   BandwidthSample sample =
-      OnPacketAcknowledgedInner(ack_time, packet_number, it->second);
-  connection_state_map_.erase(it);
+      OnPacketAcknowledgedInner(ack_time, packet_number, *sent_packet_pointer);
+  connection_state_map_.Remove(packet_number);
   return sample;
 }
 
@@ -172,23 +141,10 @@ BandwidthSample BandwidthSampler::OnPacketAcknowledgedInner(
 }
 
 void BandwidthSampler::OnPacketLost(QuicPacketNumber packet_number) {
-  if (use_new_connection_state_map_) {
-    // TODO(vasilvv): see the comment for the case of missing packets in
-    // BandwidthSampler::OnPacketAcknowledged on why this does not raise a
-    // QUIC_BUG when removal fails.
-    connection_state_map_new_.Remove(packet_number);
-    QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_faster_bandwidth_sampler, 2, 2);
-    return;
-  }
-
-  auto it = connection_state_map_.find(packet_number);
-  if (it == connection_state_map_.end()) {
-    // TODO(vasilvv): see the comment for the same case in
-    // BandwidthSampler::OnPacketAcknowledged.
-    return;
-  }
-
-  connection_state_map_.erase(it);
+  // TODO(vasilvv): see the comment for the case of missing packets in
+  // BandwidthSampler::OnPacketAcknowledged on why this does not raise a
+  // QUIC_BUG when removal fails.
+  connection_state_map_.Remove(packet_number);
 }
 
 void BandwidthSampler::OnAppLimited() {
@@ -197,17 +153,9 @@ void BandwidthSampler::OnAppLimited() {
 }
 
 void BandwidthSampler::RemoveObsoletePackets(QuicPacketNumber least_unacked) {
-  if (use_new_connection_state_map_) {
-    while (!connection_state_map_new_.IsEmpty() &&
-           connection_state_map_new_.first_packet() < least_unacked) {
-      connection_state_map_new_.Remove(
-          connection_state_map_new_.first_packet());
-    }
-    return;
-  }
-  while (!connection_state_map_.empty() &&
-         connection_state_map_.begin()->first < least_unacked) {
-    connection_state_map_.pop_front();
+  while (!connection_state_map_.IsEmpty() &&
+         connection_state_map_.first_packet() < least_unacked) {
+    connection_state_map_.Remove(connection_state_map_.first_packet());
   }
 }
 

net/quic/core/congestion_control/bandwidth_sampler.h

@@ -278,10 +278,7 @@ class QUIC_EXPORT_PRIVATE BandwidthSampler : public BandwidthSamplerInterface {
 
   // Record of the connection state at the point where each packet in flight was
   // sent, indexed by the packet number.
-  ConnectionStateMap connection_state_map_;
-  PacketNumberIndexedQueue<ConnectionStateOnSentPacket>
-      connection_state_map_new_;
-  const bool use_new_connection_state_map_;
+  PacketNumberIndexedQueue<ConnectionStateOnSentPacket> connection_state_map_;
 
   // Handles the actual bandwidth calculations, whereas the outer method handles
   // retrieving and removing |sent_packet|.

net/quic/core/congestion_control/bandwidth_sampler_test.cc

@@ -14,19 +14,12 @@ namespace test {
 class BandwidthSamplerPeer {
  public:
   static size_t GetNumberOfTrackedPackets(const BandwidthSampler& sampler) {
-    if (FLAGS_quic_reloadable_flag_quic_faster_bandwidth_sampler) {
-      return sampler.connection_state_map_new_.number_of_present_entries();
-    }
-    return sampler.connection_state_map_.size();
+    return sampler.connection_state_map_.number_of_present_entries();
   }
 
   static QuicByteCount GetPacketSize(const BandwidthSampler& sampler,
                                      QuicPacketNumber packet_number) {
-    if (FLAGS_quic_reloadable_flag_quic_faster_bandwidth_sampler) {
-      return sampler.connection_state_map_new_.GetEntry(packet_number)->size;
-    }
-    auto iterator = sampler.connection_state_map_.find(packet_number);
-    return iterator->second.size;
+    return sampler.connection_state_map_.GetEntry(packet_number)->size;
   }
 };
 

net/quic/core/congestion_control/bbr_sender.cc

@@ -85,6 +85,7 @@ BbrSender::BbrSender(const RttStats* rtt_stats,
       aggregation_epoch_start_time_(QuicTime::Zero()),
       aggregation_epoch_bytes_(0),
       bytes_acked_since_queue_drained_(0),
+      max_aggregation_bytes_multiplier_(0),
       min_rtt_(QuicTime::Delta::Zero()),
       min_rtt_timestamp_(QuicTime::Zero()),
       congestion_window_(initial_tcp_congestion_window * kDefaultTCPMSS),
@@ -199,6 +200,18 @@ void BbrSender::SetFromConfig(const QuicConfig& config,
       config.HasClientRequestedIndependentOption(kBBRR, perspective)) {
     rate_based_recovery_ = true;
   }
+  if (FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes4 &&
+      config.HasClientRequestedIndependentOption(kBBR1, perspective)) {
+    QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_bbr_ack_aggregation_bytes4, 1,
+                      2);
+    max_aggregation_bytes_multiplier_ = 1.5;
+  }
+  if (FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes4 &&
+      config.HasClientRequestedIndependentOption(kBBR2, perspective)) {
+    QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_bbr_ack_aggregation_bytes4, 2,
+                      2);
+    max_aggregation_bytes_multiplier_ = 2;
+  }
 }
 
 void BbrSender::AdjustNetworkParameters(QuicBandwidth bandwidth,
@@ -241,16 +254,7 @@ void BbrSender::OnCongestionEvent(bool /*rtt_updated*/,
         sampler_->total_bytes_acked() - total_bytes_acked_before;
 
     UpdateAckAggregationBytes(event_time, bytes_acked);
-    if (FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 ||
-        FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3) {
-      if (FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2) {
-        QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2,
-                          1, 2);
-      }
-      if (FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3) {
-        QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3,
-                          1, 2);
-      }
+    if (max_aggregation_bytes_multiplier_ > 0) {
       if (unacked_packets_->bytes_in_flight() <=
           1.25 * GetTargetCongestionWindow(pacing_gain_)) {
         bytes_acked_since_queue_drained_ = 0;
@@ -593,25 +597,15 @@ void BbrSender::CalculateCongestionWindow(QuicByteCount bytes_acked) {
   if (rtt_variance_weight_ > 0.f && !BandwidthEstimate().IsZero()) {
     target_window += rtt_variance_weight_ * rtt_stats_->mean_deviation() *
                      BandwidthEstimate();
-  } else if (FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 &&
-             is_at_full_bandwidth_) {
-    QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2, 2,
-                      2);
-    if (2 * max_ack_height_.GetBest() > bytes_acked_since_queue_drained_) {
-      target_window +=
-          2 * max_ack_height_.GetBest() - bytes_acked_since_queue_drained_;
-    }
-  } else if (FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3 &&
-             is_at_full_bandwidth_) {
-    QUIC_FLAG_COUNT_N(quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3, 2,
-                      2);
+  } else if (max_aggregation_bytes_multiplier_ > 0 && is_at_full_bandwidth_) {
     // Subtracting only half the bytes_acked_since_queue_drained ensures sending
     // doesn't completely stop for a long period of time if the queue hasn't
     // been drained recently.
-    if (1.5 * max_ack_height_.GetBest() >
+    if (max_aggregation_bytes_multiplier_ * max_ack_height_.GetBest() >
         bytes_acked_since_queue_drained_ / 2) {
-      target_window += 1.5 * max_ack_height_.GetBest() -
-                       bytes_acked_since_queue_drained_ / 2;
+      target_window +=
+          max_aggregation_bytes_multiplier_ * max_ack_height_.GetBest() -
+          bytes_acked_since_queue_drained_ / 2;
     }
   } else if (is_at_full_bandwidth_) {
     target_window += max_ack_height_.GetBest();

net/quic/core/congestion_control/bbr_sender.h

@@ -242,6 +242,10 @@ class QUIC_EXPORT_PRIVATE BbrSender : public SendAlgorithmInterface {
   // dropped below the target window.
   QuicByteCount bytes_acked_since_queue_drained_;
 
+  // The muliplier for calculating the max amount of extra CWND to add to
+  // compensate for ack aggregation.
+  float max_aggregation_bytes_multiplier_;
+
   // Minimum RTT estimate.  Automatically expires within 10 seconds (and
   // triggers PROBE_RTT mode) if no new value is sampled during that period.
   QuicTime::Delta min_rtt_;

net/quic/core/congestion_control/bbr_sender_test.cc

@@ -92,6 +92,7 @@ class BbrSenderTest : public QuicTest {
                               {&receiver_, &competing_receiver_}) {
     // These will be changed by the appropriate tests as necessary.
     FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
+    FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes4 = true;
 
     rtt_stats_ = bbr_sender_.connection()->sent_packet_manager().GetRttStats();
     sender_ = SetupBbrSender(&bbr_sender_);
@@ -239,6 +240,14 @@ class BbrSenderTest : public QuicTest {
     simulator_.RunFor(wait_time + kTestRtt);
     ASSERT_EQ(0u, bbr_sender_.bytes_to_transfer());
   }
+
+  void SetConnectionOption(QuicTag option) {
+    QuicConfig config;
+    QuicTagVector options;
+    options.push_back(option);
+    QuicConfigPeer::SetReceivedConnectionOptions(&config, options);
+    sender_->SetFromConfig(config, Perspective::IS_SERVER);
+  }
 };
 
 // Test a simple long data transfer in the default setup.
@@ -292,7 +301,6 @@ TEST_F(BbrSenderTest, SimpleTransferSmallBuffer) {
 
 // Test a simple long data transfer with 2 rtts of aggregation.
 TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes) {
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 = false;
   FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
   CreateDefaultSetup();
   // 2 RTTs of aggregation, with a max of 10kb.
@@ -318,7 +326,6 @@ TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes) {
 
 // Test a simple long data transfer with 2 rtts of aggregation.
 TEST_F(BbrSenderTest, SimpleTransferAckDecimation) {
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 = false;
   // Decrease the CWND gain so extra CWND is required with stretch acks.
   FLAGS_quic_bbr_cwnd_gain = 1.0;
   sender_ = new BbrSender(
@@ -353,72 +360,13 @@ TEST_F(BbrSenderTest, SimpleTransferAckDecimation) {
 }
 
 // Test a simple long data transfer with 2 rtts of aggregation.
-TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes2) {
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 = true;
+TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes4) {
   FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
-  CreateDefaultSetup();
-  // 2 RTTs of aggregation, with a max of 10kb.
-  EnableAggregation(10 * 1024, 2 * kTestRtt);
-
-  // Transfer 12MB.
-  DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35));
-  EXPECT_EQ(BbrSender::PROBE_BW, sender_->ExportDebugState().mode);
-  // It's possible to read a bandwidth as much as 50% too high with aggregation.
-  EXPECT_LE(kTestLinkBandwidth * 0.99f,
-            sender_->ExportDebugState().max_bandwidth);
-  // TODO(ianswett): Tighten this bound once we understand why BBR is
-  // overestimating bandwidth with aggregation. b/36022633
-  EXPECT_GE(kTestLinkBandwidth * 1.5f,
-            sender_->ExportDebugState().max_bandwidth);
-  // TODO(ianswett): Expect 0 packets are lost once BBR no longer measures
-  // bandwidth higher than the link rate.
-  // The margin here is high, because the aggregation greatly increases
-  // smoothed rtt.
-  EXPECT_GE(kTestRtt * 4, rtt_stats_->smoothed_rtt());
-  ExpectApproxEq(kTestRtt, rtt_stats_->min_rtt(), 0.33f);
-}
 
-// Test a simple long data transfer with 2 rtts of aggregation.
-TEST_F(BbrSenderTest, SimpleTransferAckDecimation2) {
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 = true;
-  // Decrease the CWND gain so extra CWND is required with stretch acks.
-  FLAGS_quic_bbr_cwnd_gain = 1.0;
-  sender_ = new BbrSender(
-      rtt_stats_,
-      QuicSentPacketManagerPeer::GetUnackedPacketMap(
-          QuicConnectionPeer::GetSentPacketManager(bbr_sender_.connection())),
-      kInitialCongestionWindowPackets, kDefaultMaxCongestionWindowPackets,
-      &random_);
-  QuicConnectionPeer::SetSendAlgorithm(bbr_sender_.connection(), sender_);
-  // Enable Ack Decimation on the receiver.
-  QuicConnectionPeer::SetAckMode(receiver_.connection(),
-                                 QuicConnection::AckMode::ACK_DECIMATION);
   CreateDefaultSetup();
+  // Enable ack aggregation that forces the queue to be drained.
+  SetConnectionOption(kBBR1);
 
-  // Transfer 12MB.
-  DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35));
-  EXPECT_EQ(BbrSender::PROBE_BW, sender_->ExportDebugState().mode);
-  // It's possible to read a bandwidth as much as 50% too high with aggregation.
-  EXPECT_LE(kTestLinkBandwidth * 0.99f,
-            sender_->ExportDebugState().max_bandwidth);
-  // TODO(ianswett): Tighten this bound once we understand why BBR is
-  // overestimating bandwidth with aggregation. b/36022633
-  EXPECT_GE(kTestLinkBandwidth * 1.5f,
-            sender_->ExportDebugState().max_bandwidth);
-  // TODO(ianswett): Expect 0 packets are lost once BBR no longer measures
-  // bandwidth higher than the link rate.
-  // The margin here is high, because the aggregation greatly increases
-  // smoothed rtt.
-  EXPECT_GE(kTestRtt * 2, rtt_stats_->smoothed_rtt());
-  ExpectApproxEq(kTestRtt, rtt_stats_->min_rtt(), 0.1f);
-}
-
-// Test a simple long data transfer with 2 rtts of aggregation.
-TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes3) {
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 = false;
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3 = true;
-  FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
-  CreateDefaultSetup();
   // 2 RTTs of aggregation, with a max of 10kb.
   EnableAggregation(10 * 1024, 2 * kTestRtt);
 
@@ -441,9 +389,7 @@ TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes3) {
 }
 
 // Test a simple long data transfer with 2 rtts of aggregation.
-TEST_F(BbrSenderTest, SimpleTransferAckDecimation3) {
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2 = false;
-  FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3 = true;
+TEST_F(BbrSenderTest, SimpleTransferAckDecimation4) {
   // Decrease the CWND gain so extra CWND is required with stretch acks.
   FLAGS_quic_bbr_cwnd_gain = 1.0;
   sender_ = new BbrSender(
@@ -457,6 +403,8 @@ TEST_F(BbrSenderTest, SimpleTransferAckDecimation3) {
   QuicConnectionPeer::SetAckMode(receiver_.connection(),
                                  QuicConnection::AckMode::ACK_DECIMATION);
   CreateDefaultSetup();
+  // Enable ack aggregation that forces the queue to be drained.
+  SetConnectionOption(kBBR1);
 
   // Transfer 12MB.
   DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35));
@@ -726,11 +674,7 @@ TEST_F(BbrSenderTest, NoBandwidthDropOnStartup) {
 TEST_F(BbrSenderTest, SimpleTransfer1RTTStartup) {
   CreateDefaultSetup();
 
-  QuicConfig config;
-  QuicTagVector options;
-  options.push_back(k1RTT);
-  QuicConfigPeer::SetReceivedConnectionOptions(&config, options);
-  sender_->SetFromConfig(config, Perspective::IS_SERVER);
+  SetConnectionOption(k1RTT);
   EXPECT_EQ(1u, sender_->num_startup_rtts());
 
   // Run until the full bandwidth is reached and check how many rounds it was.
@@ -760,11 +704,7 @@ TEST_F(BbrSenderTest, SimpleTransfer2RTTStartup) {
   FLAGS_quic_reloadable_flag_quic_bbr_add_tso_cwnd = false;
   CreateDefaultSetup();
 
-  QuicConfig config;
-  QuicTagVector options;
-  options.push_back(k2RTT);
-  QuicConfigPeer::SetReceivedConnectionOptions(&config, options);
-  sender_->SetFromConfig(config, Perspective::IS_SERVER);
+  SetConnectionOption(k2RTT);
   EXPECT_EQ(2u, sender_->num_startup_rtts());
 
   // Run until the full bandwidth is reached and check how many rounds it was.
@@ -793,11 +733,7 @@ TEST_F(BbrSenderTest, SimpleTransferLRTTStartup) {
   FLAGS_quic_reloadable_flag_quic_bbr_exit_startup_on_loss = true;
   CreateDefaultSetup();
 
-  QuicConfig config;
-  QuicTagVector options;
-  options.push_back(kLRTT);
-  QuicConfigPeer::SetReceivedConnectionOptions(&config, options);
-  sender_->SetFromConfig(config, Perspective::IS_SERVER);
+  SetConnectionOption(kLRTT);
   EXPECT_EQ(3u, sender_->num_startup_rtts());
 
   // Run until the full bandwidth is reached and check how many rounds it was.
@@ -826,11 +762,7 @@ TEST_F(BbrSenderTest, SimpleTransferLRTTStartupSmallBuffer) {
   FLAGS_quic_reloadable_flag_quic_bbr_exit_startup_on_loss = true;
   CreateSmallBufferSetup();
 
-  QuicConfig config;
-  QuicTagVector options;
-  options.push_back(kLRTT);
-  QuicConfigPeer::SetReceivedConnectionOptions(&config, options);
-  sender_->SetFromConfig(config, Perspective::IS_SERVER);
+  SetConnectionOption(kLRTT);
   EXPECT_EQ(3u, sender_->num_startup_rtts());
 
   // Run until the full bandwidth is reached and check how many rounds it was.

net/quic/core/crypto/crypto_protocol.h

@@ -84,6 +84,8 @@ const QuicTag k1RTT = TAG('1', 'R', 'T', 'T');   // STARTUP in BBR for 1 RTT
 const QuicTag k2RTT = TAG('2', 'R', 'T', 'T');   // STARTUP in BBR for 2 RTTs
 const QuicTag kLRTT = TAG('L', 'R', 'T', 'T');   // Exit STARTUP in BBR on loss
 const QuicTag kBBRR = TAG('B', 'B', 'R', 'R');   // Rate-based recovery in BBR
+const QuicTag kBBR1 = TAG('B', 'B', 'R', '1');   // Ack aggregatation v1
+const QuicTag kBBR2 = TAG('B', 'B', 'R', '2');   // Ack aggregatation v2
 const QuicTag kRENO = TAG('R', 'E', 'N', 'O');   // Reno Congestion Control
 const QuicTag kTPCC = TAG('P', 'C', 'C', '\0');  // Performance-Oriented
                                                  // Congestion Control

net/quic/core/quic_flags_list.h

@@ -88,7 +88,7 @@ QUIC_FLAG(double, FLAGS_quic_bbr_cwnd_gain, 2.0f)
 
 // If true, do not send or process stop waiting frames in QUIC if the NSTP
 // connection option is provided.
-QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_no_stop_waiting_frames, false)
+QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_no_stop_waiting_frames, true)
 
 // Allows one self address change.
 QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_allow_one_address_change, false)
@@ -160,12 +160,6 @@ QUIC_FLAG(bool,
           FLAGS_quic_reloadable_flag_quic_handle_duplicate_trailers,
           false)
 
-// Allows QUIC BBR up to twice the previously measured ack aggregation to be
-// added to the CWND as long as bytes_in_flight goes below the target recently.
-QUIC_FLAG(bool,
-          FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes2,
-          false)
-
 // If true, disables support for QUIC version 36.
 QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_disable_version_36, false)
 
@@ -173,13 +167,6 @@ QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_disable_version_36, false)
 // algorithms.
 QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_disable_packets_based_cc, false)
 
-// When enabled, adds up to 1.5x the previously measured ack aggregation in
-// bytes to the CWND, but reduces that amount by 1/2 the bytes acked since the
-// queue was drained.
-QUIC_FLAG(bool,
-          FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes3,
-          false)
-
 // When enabled, ack frame uses a deque internally instead of a set.
 QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_frames_deque, false)
 
@@ -204,3 +191,9 @@ QUIC_FLAG(bool, FLAGS_quic_restart_flag_quic_header_list_size, false)
 // When true, allows the LRTT connection option to cause QUIC BBR to exit
 // STARTUP when in recovery and there has been no bandwidth increase for 1RTT.
 QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_bbr_exit_startup_on_loss, false)
+
+// Enables the BBR1 and BBR2 QUIC connection options, which enable two forms of
+// ack aggregation that prevent persistent standing queues.
+QUIC_FLAG(bool,
+          FLAGS_quic_reloadable_flag_quic_bbr_ack_aggregation_bytes4,
+          false)

net/quic/core/quic_framer.cc

@@ -57,7 +57,7 @@ const uint8_t kPublicHeaderSequenceNumberShift = 4;
 // Special Frame Types encode both a Frame Type and corresponding flags
 // all in the Frame Type byte. Currently defined Special Frame Types are:
 // Stream             : 0b 11xxxxxx
-// Ack                : 0b 01xxxxxx
+// Ack                : 0b 101xxxxx
 //
 // Semantics of the flag bits above (the x bits) depends on the frame type.
 
@@ -66,7 +66,8 @@ const uint8_t kPublicHeaderSequenceNumberShift = 4;
 const uint8_t kQuicFrameTypeSpecialMask = 0xE0;  // 0b 11100000
 const uint8_t kQuicFrameTypeStreamMask_Pre40 = 0x80;
 const uint8_t kQuicFrameTypeStreamMask = 0xC0;
-const uint8_t kQuicFrameTypeAckMask = 0x40;
+const uint8_t kQuicFrameTypeAckMask_Pre40 = 0x40;
+const uint8_t kQuicFrameTypeAckMask = 0xA0;
 
 // Stream type format is 11FSSOOD.
 // Stream frame relative shifts and masks for interpreting the stream flags.
@@ -96,7 +97,7 @@ const uint8_t kQuicSequenceNumberLengthShift = 2;
 
 // Acks may have only one ack block.
 const uint8_t kQuicHasMultipleAckBlocksMask = 0x01;
-const uint8_t kQuicHasMultipleAckBlocksShift = 1;
+const uint8_t kQuicHasMultipleAckBlocksShift_Pre40 = 1;
 
 // Returns the absolute value of the difference between |a| and |b|.
 QuicPacketNumber Delta(QuicPacketNumber a, QuicPacketNumber b) {
@@ -1005,7 +1006,11 @@ bool QuicFramer::ProcessFrameData(QuicDataReader* reader,
       }
 
       // Ack Frame
-      if (frame_type & kQuicFrameTypeAckMask) {
+      if ((quic_version_ < QUIC_VERSION_40 &&
+           (frame_type & kQuicFrameTypeAckMask_Pre40)) ||
+          (quic_version_ >= QUIC_VERSION_40 &&
+           ((frame_type & kQuicFrameTypeSpecialMask) ==
+            kQuicFrameTypeAckMask))) {
         QuicAckFrame frame;
         if (!ProcessAckFrame(reader, frame_type, &frame)) {
           return RaiseError(QUIC_INVALID_ACK_DATA);
@@ -1145,6 +1150,10 @@ bool QuicFramer::ProcessFrameData(QuicDataReader* reader,
   return true;
 }
 
+uint8_t ExtractBits(uint8_t flags, uint8_t num_bits, uint8_t offset) {
+  return ((flags >> offset) & ~(0xFF << num_bits));
+}
+
 bool QuicFramer::ProcessStreamFrame(QuicDataReader* reader,
                                     uint8_t frame_type,
                                     QuicStreamFrame* frame) {
@@ -1245,14 +1254,17 @@ bool QuicFramer::ProcessAckFrame(QuicDataReader* reader,
                                  QuicAckFrame* ack_frame) {
   // Determine the two lengths from the frame type: largest acked length,
   // ack block length.
+  uint8_t offset = 0u;
   const QuicPacketNumberLength ack_block_length =
-      ReadSequenceNumberLength(frame_type);
-  frame_type >>= kQuicSequenceNumberLengthShift;
+      ReadSequenceNumberLength(ExtractBits(frame_type, 2, offset));
+  offset += kQuicSequenceNumberLengthShift;
   const QuicPacketNumberLength largest_acked_length =
-      ReadSequenceNumberLength(frame_type);
-  frame_type >>= kQuicSequenceNumberLengthShift;
-  frame_type >>= kQuicHasMultipleAckBlocksShift;
-  bool has_ack_blocks = frame_type & kQuicHasMultipleAckBlocksMask;
+      ReadSequenceNumberLength(ExtractBits(frame_type, 2, offset));
+  offset += kQuicSequenceNumberLengthShift;
+  if (quic_version_ < QUIC_VERSION_40) {
+    offset += kQuicHasMultipleAckBlocksShift_Pre40;
+  }
+  bool has_ack_blocks = ExtractBits(frame_type, 1, offset) != 0;
 
   if (!reader->ReadBytesToUInt64(largest_acked_length,
                                  &ack_frame->largest_observed)) {
@@ -1975,8 +1987,9 @@ bool QuicFramer::AppendAckFrameAndTypeByte(const QuicAckFrame& frame,
   // Whether there are multiple ack blocks.
   uint8_t type_byte =
       new_ack_info.num_ack_blocks == 0 ? 0 : kQuicHasMultipleAckBlocksMask;
-  type_byte <<= kQuicHasMultipleAckBlocksShift;
-
+  if (quic_version_ < QUIC_VERSION_40) {
+    type_byte <<= kQuicHasMultipleAckBlocksShift_Pre40;
+  }
   // Largest acked length.
   type_byte <<= kQuicSequenceNumberLengthShift;
   type_byte |= GetPacketNumberFlags(largest_acked_length);
@@ -1985,7 +1998,11 @@ bool QuicFramer::AppendAckFrameAndTypeByte(const QuicAckFrame& frame,
   type_byte <<= kQuicSequenceNumberLengthShift;
   type_byte |= GetPacketNumberFlags(ack_block_length);
 
-  type_byte |= kQuicFrameTypeAckMask;
+  if (quic_version_ < QUIC_VERSION_40) {
+    type_byte |= kQuicFrameTypeAckMask_Pre40;
+  } else {
+    type_byte |= kQuicFrameTypeAckMask;
+  }
 
   if (!writer->WriteUInt8(type_byte)) {
     return false;

net/quic/test_tools/quic_sent_packet_manager_peer.cc

@@ -82,17 +82,6 @@ bool QuicSentPacketManagerPeer::HasPendingPackets(
   return sent_packet_manager->unacked_packets_.HasInFlightPackets();
 }
 
-// static
-QuicTime QuicSentPacketManagerPeer::GetSentTime(
-    const QuicSentPacketManager* sent_packet_manager,
-    QuicPacketNumber packet_number) {
-  DCHECK(sent_packet_manager->unacked_packets_.IsUnacked(packet_number));
-
-  return sent_packet_manager->unacked_packets_
-      .GetTransmissionInfo(packet_number)
-      .sent_time;
-}
-
 // static
 bool QuicSentPacketManagerPeer::IsRetransmission(
     QuicSentPacketManager* sent_packet_manager,
@@ -149,13 +138,6 @@ QuicByteCount QuicSentPacketManagerPeer::GetBytesInFlight(
   return sent_packet_manager->unacked_packets_.bytes_in_flight();
 }
 
-// static
-QuicSentPacketManager::NetworkChangeVisitor*
-QuicSentPacketManagerPeer::GetNetworkChangeVisitor(
-    const QuicSentPacketManager* sent_packet_manager) {
-  return sent_packet_manager->network_change_visitor_;
-}
-
 // static
 void QuicSentPacketManagerPeer::SetConsecutiveRtoCount(
     QuicSentPacketManager* sent_packet_manager,

net/quic/test_tools/quic_sent_packet_manager_peer.h

@@ -48,9 +48,6 @@ class QuicSentPacketManagerPeer {
   static bool HasPendingPackets(
       const QuicSentPacketManager* sent_packet_manager);
 
-  static QuicTime GetSentTime(const QuicSentPacketManager* sent_packet_manager,
-                              QuicPacketNumber packet_number);
-
   // Returns true if |packet_number| is a retransmission of a packet.
   static bool IsRetransmission(QuicSentPacketManager* sent_packet_manager,
                                QuicPacketNumber packet_number);
@@ -71,9 +68,6 @@ class QuicSentPacketManagerPeer {
   static QuicByteCount GetBytesInFlight(
       const QuicSentPacketManager* sent_packet_manager);
 
-  static QuicSentPacketManager::NetworkChangeVisitor* GetNetworkChangeVisitor(
-      const QuicSentPacketManager* sent_packet_manager);
-
   static void SetConsecutiveRtoCount(QuicSentPacketManager* sent_packet_manager,
                                      size_t count);