Internal QUICHE change
PiperOrigin-RevId: 296522161
Change-Id: I078e53c1ad6b63b052fc1f0993ebfd5097aa69dd
diff --git a/quic/core/congestion_control/bandwidth_sampler.cc b/quic/core/congestion_control/bandwidth_sampler.cc
index 6272664..1a56e62 100644
--- a/quic/core/congestion_control/bandwidth_sampler.cc
+++ b/quic/core/congestion_control/bandwidth_sampler.cc
@@ -32,18 +32,19 @@
// Reset the current aggregation epoch as soon as the ack arrival rate is less
// than or equal to the max bandwidth.
if (aggregation_epoch_bytes_ <=
- GetQuicFlag(FLAGS_quic_ack_aggregation_bandwidth_threshold) *
- expected_bytes_acked) {
+ ack_aggregation_bandwidth_threshold_ * expected_bytes_acked) {
QUIC_DVLOG(3) << "Starting a new aggregation epoch because "
"aggregation_epoch_bytes_ "
<< aggregation_epoch_bytes_
<< " is smaller than expected. "
- "quic_ack_aggregation_bandwidth_threshold:"
- << GetQuicFlag(FLAGS_quic_ack_aggregation_bandwidth_threshold)
+ "ack_aggregation_bandwidth_threshold_:"
+ << ack_aggregation_bandwidth_threshold_
<< ", expected_bytes_acked:" << expected_bytes_acked
<< ", bandwidth_estimate:" << bandwidth_estimate
<< ", aggregation_duration:"
- << (ack_time - aggregation_epoch_start_time_);
+ << (ack_time - aggregation_epoch_start_time_)
+ << ", new_aggregation_epoch:" << ack_time
+ << ", new_aggregation_bytes_acked:" << bytes_acked;
// Reset to start measuring a new aggregation epoch.
aggregation_epoch_bytes_ = bytes_acked;
aggregation_epoch_start_time_ = ack_time;
@@ -82,7 +83,8 @@
max_tracked_packets_(GetQuicFlag(FLAGS_quic_max_tracked_packet_count)),
unacked_packet_map_(unacked_packet_map),
max_ack_height_tracker_(max_height_tracker_window_length),
- total_bytes_acked_after_last_ack_event_(0) {
+ total_bytes_acked_after_last_ack_event_(0),
+ overestimate_avoidance_(false) {
if (remove_packets_once_per_congestion_event_) {
QUIC_RELOADABLE_FLAG_COUNT(
quic_bw_sampler_remove_packets_once_per_congestion_event2);
@@ -92,6 +94,17 @@
}
}
+void BandwidthSampler::EnableOverestimateAvoidance() {
+ if (overestimate_avoidance_) {
+ return;
+ }
+
+ overestimate_avoidance_ = true;
+ // TODO(wub): Change the default value of
+ // --quic_ack_aggregation_bandwidth_threshold to 2.0.
+ max_ack_height_tracker_.SetAckAggregationBandwidthThreshold(2.0);
+}
+
BandwidthSampler::~BandwidthSampler() {}
void BandwidthSampler::OnPacketSent(
@@ -116,6 +129,12 @@
// importantly at the beginning of the connection.
if (bytes_in_flight == 0) {
last_acked_packet_ack_time_ = sent_time;
+ if (overestimate_avoidance_) {
+ recent_ack_points_.Clear();
+ recent_ack_points_.Update(sent_time, total_bytes_acked_);
+ a0_candidates_.clear();
+ a0_candidates_.push_back(recent_ack_points_.MostRecentPoint());
+ }
total_bytes_sent_at_last_acked_packet_ = total_bytes_sent_;
// In this situation ack compression is not a concern, set send rate to
@@ -240,9 +259,17 @@
}
total_bytes_acked_after_last_ack_event_ = total_bytes_acked_;
- return max_ack_height_tracker_.Update(bandwidth_estimate, round_trip_count,
- last_acked_packet_ack_time_,
- newly_acked_bytes);
+ QuicByteCount extra_acked = max_ack_height_tracker_.Update(
+ bandwidth_estimate, round_trip_count, last_acked_packet_ack_time_,
+ newly_acked_bytes);
+ // If |extra_acked| is zero, i.e. this ack event marks the start of a new ack
+ // aggregation epoch, save LessRecentPoint, which is the last ack point of the
+ // previous epoch, as a A0 candidate.
+ if (overestimate_avoidance_ && extra_acked == 0) {
+ a0_candidates_.push_back(recent_ack_points_.LessRecentPoint());
+ QUIC_DVLOG(1) << "New a0_candidate:" << a0_candidates_.back();
+ }
+ return extra_acked;
}
BandwidthSample BandwidthSampler::OnPacketAcknowledged(
@@ -271,6 +298,9 @@
sent_packet.send_time_state.total_bytes_sent;
last_acked_packet_sent_time_ = sent_packet.sent_time;
last_acked_packet_ack_time_ = ack_time;
+ if (overestimate_avoidance_) {
+ recent_ack_points_.Update(ack_time, total_bytes_acked_);
+ }
// Exit app-limited phase once a packet that was sent while the connection is
// not app-limited is acknowledged.
@@ -296,13 +326,22 @@
sent_packet.sent_time - sent_packet.last_acked_packet_sent_time);
}
+ AckPoint a0;
+ if (overestimate_avoidance_ &&
+ ChooseA0Point(sent_packet.send_time_state.total_bytes_acked, &a0)) {
+ QUIC_DVLOG(2) << "Using a0 point: " << a0;
+ } else {
+ a0.ack_time = sent_packet.last_acked_packet_ack_time,
+ a0.total_bytes_acked = sent_packet.send_time_state.total_bytes_acked;
+ }
+
// During the slope calculation, ensure that ack time of the current packet is
// always larger than the time of the previous packet, otherwise division by
// zero or integer underflow can occur.
- if (ack_time <= sent_packet.last_acked_packet_ack_time) {
+ if (ack_time <= a0.ack_time) {
// TODO(wub): Compare this code count before and after fixing clock jitter
// issue.
- if (sent_packet.last_acked_packet_ack_time == sent_packet.sent_time) {
+ if (a0.ack_time == sent_packet.sent_time) {
// This is the 1st packet after quiescense.
QUIC_CODE_COUNT_N(quic_prev_ack_time_larger_than_current_ack_time, 1, 2);
} else {
@@ -310,14 +349,13 @@
}
QUIC_LOG_EVERY_N_SEC(ERROR, 60)
<< "Time of the previously acked packet:"
- << sent_packet.last_acked_packet_ack_time.ToDebuggingValue()
+ << a0.ack_time.ToDebuggingValue()
<< " is larger than the ack time of the current packet:"
<< ack_time.ToDebuggingValue();
return BandwidthSample();
}
QuicBandwidth ack_rate = QuicBandwidth::FromBytesAndTimeDelta(
- total_bytes_acked_ - sent_packet.send_time_state.total_bytes_acked,
- ack_time - sent_packet.last_acked_packet_ack_time);
+ total_bytes_acked_ - a0.total_bytes_acked, ack_time - a0.ack_time);
BandwidthSample sample;
sample.bandwidth = std::min(send_rate, ack_rate);
@@ -329,6 +367,35 @@
return sample;
}
+bool BandwidthSampler::ChooseA0Point(QuicByteCount total_bytes_acked,
+ AckPoint* a0) {
+ if (a0_candidates_.empty()) {
+ QUIC_BUG << "No A0 point candicates. total_bytes_acked:"
+ << total_bytes_acked;
+ return false;
+ }
+
+ if (a0_candidates_.size() == 1) {
+ *a0 = a0_candidates_.front();
+ return true;
+ }
+
+ for (size_t i = 1; i < a0_candidates_.size(); ++i) {
+ if (a0_candidates_[i].total_bytes_acked > total_bytes_acked) {
+ *a0 = a0_candidates_[i - 1];
+ if (i > 1) {
+ a0_candidates_.pop_front_n(i - 1);
+ }
+ return true;
+ }
+ }
+
+ // All candidates' total_bytes_acked is <= |total_bytes_acked|.
+ *a0 = a0_candidates_.back();
+ a0_candidates_.pop_front_n(a0_candidates_.size() - 1);
+ return true;
+}
+
SendTimeState BandwidthSampler::OnPacketLost(QuicPacketNumber packet_number,
QuicPacketLength bytes_lost) {
// TODO(vasilvv): see the comment for the case of missing packets in
diff --git a/quic/core/congestion_control/bandwidth_sampler.h b/quic/core/congestion_control/bandwidth_sampler.h
index 90b20fc..210a219 100644
--- a/quic/core/congestion_control/bandwidth_sampler.h
+++ b/quic/core/congestion_control/bandwidth_sampler.h
@@ -9,6 +9,7 @@
#include "net/third_party/quiche/src/quic/core/congestion_control/windowed_filter.h"
#include "net/third_party/quiche/src/quic/core/packet_number_indexed_queue.h"
#include "net/third_party/quiche/src/quic/core/quic_bandwidth.h"
+#include "net/third_party/quiche/src/quic/core/quic_circular_deque.h"
#include "net/third_party/quiche/src/quic/core/quic_packets.h"
#include "net/third_party/quiche/src/quic/core/quic_time.h"
#include "net/third_party/quiche/src/quic/core/quic_types.h"
@@ -110,6 +111,14 @@
max_ack_height_filter_.Reset(new_height, new_time);
}
+ void SetAckAggregationBandwidthThreshold(double threshold) {
+ ack_aggregation_bandwidth_threshold_ = threshold;
+ }
+
+ double ack_aggregation_bandwidth_threshold() const {
+ return ack_aggregation_bandwidth_threshold_;
+ }
+
uint64_t num_ack_aggregation_epochs() const {
return num_ack_aggregation_epochs_;
}
@@ -130,6 +139,8 @@
// The number of ack aggregation epochs ever started, including the ongoing
// one. Stats only.
uint64_t num_ack_aggregation_epochs_ = 0;
+ double ack_aggregation_bandwidth_threshold_ =
+ GetQuicFlag(FLAGS_quic_ack_aggregation_bandwidth_threshold);
};
// An interface common to any class that can provide bandwidth samples from the
@@ -245,8 +256,8 @@
// For that purpose, BandwidthSampler always keeps track of the most recently
// acknowledged packet, and records it together with every outgoing packet.
// When a packet gets acknowledged (A_1), it has not only information about when
-// it itself was sent (S_1), but also the information about the latest
-// acknowledged packet right before it was sent (S_0 and A_0).
+// it itself was sent (S_1), but also the information about a previously
+// acknowledged packet before it was sent (S_0 and A_0).
//
// Based on that data, send and ack rate are estimated as:
// send_rate = (bytes(S_1) - bytes(S_0)) / (time(S_1) - time(S_0))
@@ -362,6 +373,57 @@
return one_bw_sample_per_ack_event_;
}
+ // AckPoint represents a point on the ack line.
+ struct QUIC_NO_EXPORT AckPoint {
+ QuicTime ack_time = QuicTime::Zero();
+ QuicByteCount total_bytes_acked = 0;
+
+ friend QUIC_NO_EXPORT std::ostream& operator<<(std::ostream& os,
+ const AckPoint& ack_point) {
+ return os << ack_point.ack_time << ":" << ack_point.total_bytes_acked;
+ }
+ };
+
+ // RecentAckPoints maintains the most recent 2 ack points at distinct times.
+ class QUIC_NO_EXPORT RecentAckPoints {
+ public:
+ void Update(QuicTime ack_time, QuicByteCount total_bytes_acked) {
+ DCHECK_GE(total_bytes_acked, ack_points_[1].total_bytes_acked);
+
+ if (ack_time < ack_points_[1].ack_time) {
+ // This can only happen when time goes backwards, we use the smaller
+ // timestamp for the most recent ack point in that case.
+ // TODO(wub): Add a QUIC_BUG if ack time stops going backwards.
+ ack_points_[1].ack_time = ack_time;
+ } else if (ack_time > ack_points_[1].ack_time) {
+ ack_points_[0] = ack_points_[1];
+ ack_points_[1].ack_time = ack_time;
+ }
+
+ ack_points_[1].total_bytes_acked = total_bytes_acked;
+ }
+
+ void Clear() { ack_points_[0] = ack_points_[1] = AckPoint(); }
+
+ const AckPoint& MostRecentPoint() const { return ack_points_[1]; }
+
+ const AckPoint& LessRecentPoint() const {
+ if (ack_points_[0].total_bytes_acked != 0) {
+ return ack_points_[0];
+ }
+
+ return ack_points_[1];
+ }
+
+ private:
+ AckPoint ack_points_[2];
+ };
+
+ void EnableOverestimateAvoidance();
+ bool IsOverestimateAvoidanceEnabled() const {
+ return overestimate_avoidance_;
+ }
+
private:
friend class test::BandwidthSamplerPeer;
@@ -428,6 +490,20 @@
void SentPacketToSendTimeState(const ConnectionStateOnSentPacket& sent_packet,
SendTimeState* send_time_state) const;
+ // Choose the best a0 from |a0_candidates_| to calculate the ack rate.
+ // |total_bytes_acked| is the total bytes acked when the packet being acked is
+ // sent. The best a0 is chosen as follows:
+ // - If there's only one candidate, use it.
+ // - If there are multiple candidates, let a[n] be the nth candidate, and
+ // a[n-1].total_bytes_acked <= |total_bytes_acked| < a[n].total_bytes_acked,
+ // use a[n-1].
+ // - If all candidates's total_bytes_acked is > |total_bytes_acked|, use a[0].
+ // This may happen when acks are received out of order, and ack[n] caused
+ // some candidates of ack[n-x] to be removed.
+ // - If all candidates's total_bytes_acked is <= |total_bytes_acked|, use
+ // a[a.size()-1].
+ bool ChooseA0Point(QuicByteCount total_bytes_acked, AckPoint* a0);
+
// The total number of congestion controlled bytes sent during the connection.
QuicByteCount total_bytes_sent_;
@@ -466,6 +542,9 @@
// sent, indexed by the packet number.
PacketNumberIndexedQueue<ConnectionStateOnSentPacket> connection_state_map_;
+ RecentAckPoints recent_ack_points_;
+ QuicCircularDeque<AckPoint> a0_candidates_;
+
// Maximum number of tracked packets.
const QuicPacketCount max_tracked_packets_;
@@ -493,6 +572,10 @@
const bool one_bw_sample_per_ack_event_ =
remove_packets_once_per_congestion_event_ &&
GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event2);
+
+ // True if --quic_avoid_overestimate_bandwidth_with_aggregation=true and
+ // connection option 'BSAO' is set.
+ bool overestimate_avoidance_;
};
} // namespace quic
diff --git a/quic/core/congestion_control/bandwidth_sampler_test.cc b/quic/core/congestion_control/bandwidth_sampler_test.cc
index 13fa1c0..2529ba1 100644
--- a/quic/core/congestion_control/bandwidth_sampler_test.cc
+++ b/quic/core/congestion_control/bandwidth_sampler_test.cc
@@ -45,6 +45,10 @@
round_trip_count_(0) {
// Ensure that the clock does not start at zero.
clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1));
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ sampler_.EnableOverestimateAvoidance();
+ }
}
MockClock clock_;
@@ -286,7 +290,7 @@
QuicBandwidth last_bandwidth = QuicBandwidth::Zero();
for (int i = 21; i <= 40; i++) {
last_bandwidth = AckPacket(i);
- EXPECT_EQ(expected_bandwidth, last_bandwidth);
+ EXPECT_EQ(expected_bandwidth, last_bandwidth) << "i is " << i;
clock_.AdvanceTime(time_between_packets);
}
if (sampler_.remove_packets_once_per_congestion_event()) {
@@ -690,7 +694,7 @@
// Send and ack packet 2, 3 and 4.
round_trip_count_++;
- est_bandwidth_upper_bound_ = first_packet_sending_rate * 0.9;
+ est_bandwidth_upper_bound_ = first_packet_sending_rate * 0.3;
SendPacket(2);
SendPacket(3);
SendPacket(4);
@@ -704,7 +708,12 @@
class MaxAckHeightTrackerTest : public QuicTest {
protected:
- MaxAckHeightTrackerTest() : tracker_(/*initial_filter_window=*/10) {}
+ MaxAckHeightTrackerTest() : tracker_(/*initial_filter_window=*/10) {
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ tracker_.SetAckAggregationBandwidthThreshold(1.8);
+ }
+ }
// Run a full aggregation episode, which is one or more aggregated acks,
// followed by a quiet period in which no ack happens.
@@ -787,9 +796,15 @@
1200, true);
now_ = now_ - QuicTime::Delta::FromMilliseconds(1);
- AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6),
- 1200, false);
- EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) {
+ AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6),
+ 1200, true);
+ EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs());
+ } else {
+ AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6),
+ 1200, false);
+ EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ }
}
TEST_F(MaxAckHeightTrackerTest, VeryAggregatedSmallAcks) {
@@ -799,9 +814,15 @@
true);
now_ = now_ - QuicTime::Delta::FromMilliseconds(1);
- AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6), 300,
- false);
- EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) {
+ AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6),
+ 300, true);
+ EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs());
+ } else {
+ AggregationEpisode(bandwidth_ * 20, QuicTime::Delta::FromMilliseconds(6),
+ 300, false);
+ EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ }
}
TEST_F(MaxAckHeightTrackerTest, SomewhatAggregatedLargeAck) {
@@ -811,9 +832,15 @@
1000, true);
now_ = now_ - QuicTime::Delta::FromMilliseconds(1);
- AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50),
- 1000, false);
- EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) {
+ AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50),
+ 1000, true);
+ EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs());
+ } else {
+ AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50),
+ 1000, false);
+ EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ }
}
TEST_F(MaxAckHeightTrackerTest, SomewhatAggregatedSmallAcks) {
@@ -823,9 +850,15 @@
true);
now_ = now_ - QuicTime::Delta::FromMilliseconds(1);
- AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50), 100,
- false);
- EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ if (tracker_.ack_aggregation_bandwidth_threshold() > 1.1) {
+ AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50),
+ 100, true);
+ EXPECT_EQ(3u, tracker_.num_ack_aggregation_epochs());
+ } else {
+ AggregationEpisode(bandwidth_ * 2, QuicTime::Delta::FromMilliseconds(50),
+ 100, false);
+ EXPECT_EQ(2u, tracker_.num_ack_aggregation_epochs());
+ }
}
TEST_F(MaxAckHeightTrackerTest, NotAggregated) {
diff --git a/quic/core/congestion_control/bbr2_misc.h b/quic/core/congestion_control/bbr2_misc.h
index 2226566..191790b 100644
--- a/quic/core/congestion_control/bbr2_misc.h
+++ b/quic/core/congestion_control/bbr2_misc.h
@@ -359,6 +359,10 @@
return bandwidth_sampler_.max_ack_height();
}
+ void EnableOverestimateAvoidance() {
+ bandwidth_sampler_.EnableOverestimateAvoidance();
+ }
+
void OnPacketNeutered(QuicPacketNumber packet_number) {
bandwidth_sampler_.OnPacketNeutered(packet_number);
}
diff --git a/quic/core/congestion_control/bbr2_sender.cc b/quic/core/congestion_control/bbr2_sender.cc
index 108cb34..60fb374 100644
--- a/quic/core/congestion_control/bbr2_sender.cc
+++ b/quic/core/congestion_control/bbr2_sender.cc
@@ -93,6 +93,13 @@
if (config.HasClientRequestedIndependentOption(kBBR9, perspective)) {
flexible_app_limited_ = true;
}
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation) &&
+ config.HasClientRequestedIndependentOption(kBSAO, perspective)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(
+ quic_avoid_overestimate_bandwidth_with_aggregation, 4, 4);
+ model_.EnableOverestimateAvoidance();
+ }
}
Limits<QuicByteCount> Bbr2Sender::GetCwndLimitsByMode() const {
diff --git a/quic/core/congestion_control/bbr2_simulator_test.cc b/quic/core/congestion_control/bbr2_simulator_test.cc
index d109d54..e80b5ac 100644
--- a/quic/core/congestion_control/bbr2_simulator_test.cc
+++ b/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -16,6 +16,7 @@
#include "net/third_party/quiche/src/quic/platform/api/quic_logging.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_optional.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_test.h"
+#include "net/third_party/quiche/src/quic/test_tools/quic_config_peer.h"
#include "net/third_party/quiche/src/quic/test_tools/quic_connection_peer.h"
#include "net/third_party/quiche/src/quic/test_tools/quic_sent_packet_manager_peer.h"
#include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h"
@@ -304,6 +305,14 @@
aggregation_timeout);
}
+ void SetConnectionOption(QuicTag option) {
+ QuicConfig config;
+ QuicTagVector options;
+ options.push_back(option);
+ QuicConfigPeer::SetReceivedConnectionOptions(&config, options);
+ sender_->SetFromConfig(config, Perspective::IS_SERVER);
+ }
+
bool Bbr2ModeIsOneOf(const std::vector<Bbr2Mode>& expected_modes) const {
const Bbr2Mode mode = sender_->ExportDebugState().mode;
for (Bbr2Mode expected_mode : expected_modes) {
@@ -416,6 +425,10 @@
}
TEST_F(Bbr2DefaultTopologyTest, SimpleTransfer2RTTAggregationBytes) {
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ SetConnectionOption(kBSAO);
+ }
DefaultTopologyParams params;
CreateNetwork(params);
// 2 RTTs of aggregation, with a max of 10kb.
@@ -425,11 +438,17 @@
DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35));
EXPECT_TRUE(Bbr2ModeIsOneOf({Bbr2Mode::PROBE_BW, Bbr2Mode::PROBE_RTT}));
- EXPECT_LE(params.BottleneckBandwidth() * 0.99f,
- sender_->ExportDebugState().bandwidth_hi);
- // TODO(b/36022633): Bandwidth sampler overestimates with aggregation.
- EXPECT_GE(params.BottleneckBandwidth() * 1.5f,
- sender_->ExportDebugState().bandwidth_hi);
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_APPROX_EQ(params.BottleneckBandwidth(),
+ sender_->ExportDebugState().bandwidth_hi, 0.01f);
+ } else {
+ EXPECT_LE(params.BottleneckBandwidth() * 0.99f,
+ sender_->ExportDebugState().bandwidth_hi);
+ // TODO(b/36022633): Bandwidth sampler overestimates with aggregation.
+ EXPECT_GE(params.BottleneckBandwidth() * 1.5f,
+ sender_->ExportDebugState().bandwidth_hi);
+ }
EXPECT_LE(sender_loss_rate_in_packets(), 0.05);
// The margin here is high, because the aggregation greatly increases
// smoothed rtt.
@@ -438,6 +457,10 @@
}
TEST_F(Bbr2DefaultTopologyTest, SimpleTransferAckDecimation) {
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ SetConnectionOption(kBSAO);
+ }
// Enable Ack Decimation on the receiver.
QuicConnectionPeer::SetAckMode(receiver_endpoint_.connection(),
AckMode::ACK_DECIMATION);
@@ -448,11 +471,17 @@
DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35));
EXPECT_TRUE(Bbr2ModeIsOneOf({Bbr2Mode::PROBE_BW, Bbr2Mode::PROBE_RTT}));
- EXPECT_LE(params.BottleneckBandwidth() * 0.99f,
- sender_->ExportDebugState().bandwidth_hi);
- // TODO(b/36022633): Bandwidth sampler overestimates with aggregation.
- EXPECT_GE(params.BottleneckBandwidth() * 1.1f,
- sender_->ExportDebugState().bandwidth_hi);
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_APPROX_EQ(params.BottleneckBandwidth(),
+ sender_->ExportDebugState().bandwidth_hi, 0.01f);
+ } else {
+ EXPECT_LE(params.BottleneckBandwidth() * 0.99f,
+ sender_->ExportDebugState().bandwidth_hi);
+ // TODO(b/36022633): Bandwidth sampler overestimates with aggregation.
+ EXPECT_GE(params.BottleneckBandwidth() * 1.1f,
+ sender_->ExportDebugState().bandwidth_hi);
+ }
EXPECT_LE(sender_loss_rate_in_packets(), 0.001);
EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
// The margin here is high, because the aggregation greatly increases
diff --git a/quic/core/congestion_control/bbr_sender.cc b/quic/core/congestion_control/bbr_sender.cc
index da723c4..fba16b5 100644
--- a/quic/core/congestion_control/bbr_sender.cc
+++ b/quic/core/congestion_control/bbr_sender.cc
@@ -330,6 +330,13 @@
max_congestion_window_with_network_parameters_adjusted_ =
100 * kDefaultTCPMSS;
}
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation) &&
+ config.HasClientRequestedIndependentOption(kBSAO, perspective)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(
+ quic_avoid_overestimate_bandwidth_with_aggregation, 3, 4);
+ sampler_.EnableOverestimateAvoidance();
+ }
}
void BbrSender::AdjustNetworkParameters(const NetworkParams& params) {
diff --git a/quic/core/congestion_control/bbr_sender_test.cc b/quic/core/congestion_control/bbr_sender_test.cc
index 096b7fa..2b262c9 100644
--- a/quic/core/congestion_control/bbr_sender_test.cc
+++ b/quic/core/congestion_control/bbr_sender_test.cc
@@ -430,20 +430,32 @@
// Flaky in Chromium: https://crbug.com/1042575
TEST_F(BbrSenderTest,
QUIC_TEST_DISABLED_IN_CHROME(SimpleTransfer2RTTAggregationBytes)) {
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ SetConnectionOption(kBSAO);
+ }
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);
+ EXPECT_TRUE(sender_->ExportDebugState().mode == BbrSender::PROBE_BW ||
+ sender_->ExportDebugState().mode == BbrSender::PROBE_RTT);
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_APPROX_EQ(kTestLinkBandwidth,
+ sender_->ExportDebugState().max_bandwidth, 0.01f);
+ } else {
+ // It's possible to read a bandwidth as much as 50% too high with
+ // aggregation.
+ EXPECT_LE(kTestLinkBandwidth * 0.93f,
+ 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);
+ }
// The margin here is high, because the aggregation greatly increases
// smoothed rtt.
EXPECT_GE(kTestRtt * 4, rtt_stats_->smoothed_rtt());
@@ -452,6 +464,10 @@
// Test a simple long data transfer with 2 rtts of aggregation.
TEST_F(BbrSenderTest, SimpleTransferAckDecimation) {
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ SetConnectionOption(kBSAO);
+ }
// Decrease the CWND gain so extra CWND is required with stretch acks.
SetQuicFlag(FLAGS_quic_bbr_cwnd_gain, 1.0);
sender_ = new BbrSender(
@@ -470,13 +486,21 @@
// 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);
+
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_APPROX_EQ(kTestLinkBandwidth,
+ sender_->ExportDebugState().max_bandwidth, 0.01f);
+ } else {
+ // It's possible to read a bandwidth as much as 50% too high with
+ // aggregation.
+ EXPECT_LE(kTestLinkBandwidth * 0.93f,
+ 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.
EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
@@ -488,6 +512,10 @@
// Test a simple long data transfer with 2 rtts of aggregation.
TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes20RTTWindow) {
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ SetConnectionOption(kBSAO);
+ }
// Disable Ack Decimation on the receiver, because it can increase srtt.
QuicConnectionPeer::SetAckMode(receiver_.connection(), AckMode::TCP_ACKING);
CreateDefaultSetup();
@@ -499,13 +527,20 @@
DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35));
EXPECT_TRUE(sender_->ExportDebugState().mode == BbrSender::PROBE_BW ||
sender_->ExportDebugState().mode == BbrSender::PROBE_RTT);
- // 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);
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_APPROX_EQ(kTestLinkBandwidth,
+ sender_->ExportDebugState().max_bandwidth, 0.01f);
+ } else {
+ // It's possible to read a bandwidth as much as 50% too high with
+ // aggregation.
+ EXPECT_LE(kTestLinkBandwidth * 0.93f,
+ 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
@@ -516,6 +551,10 @@
// Test a simple long data transfer with 2 rtts of aggregation.
TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes40RTTWindow) {
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ SetConnectionOption(kBSAO);
+ }
// Disable Ack Decimation on the receiver, because it can increase srtt.
QuicConnectionPeer::SetAckMode(receiver_.connection(), AckMode::TCP_ACKING);
CreateDefaultSetup();
@@ -527,13 +566,20 @@
DoSimpleTransfer(12 * 1024 * 1024, QuicTime::Delta::FromSeconds(35));
EXPECT_TRUE(sender_->ExportDebugState().mode == BbrSender::PROBE_BW ||
sender_->ExportDebugState().mode == BbrSender::PROBE_RTT);
- // 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);
+ if (GetQuicReloadableFlag(
+ quic_avoid_overestimate_bandwidth_with_aggregation)) {
+ EXPECT_APPROX_EQ(kTestLinkBandwidth,
+ sender_->ExportDebugState().max_bandwidth, 0.01f);
+ } else {
+ // It's possible to read a bandwidth as much as 50% too high with
+ // aggregation.
+ EXPECT_LE(kTestLinkBandwidth * 0.93f,
+ 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
