Do not add lumpy tokens to QUIC's PacingSender when CWND limited.
Protected by quic_reloadable_flag_quic_fix_pacing_sender_bursts.
PiperOrigin-RevId: 378501542
diff --git a/quic/core/congestion_control/bbr2_simulator_test.cc b/quic/core/congestion_control/bbr2_simulator_test.cc
index 99acf2c..7216eea 100644
--- a/quic/core/congestion_control/bbr2_simulator_test.cc
+++ b/quic/core/congestion_control/bbr2_simulator_test.cc
@@ -477,7 +477,11 @@
EXPECT_APPROX_EQ(params.BottleneckBandwidth(),
sender_->ExportDebugState().bandwidth_hi, 0.01f);
- EXPECT_LE(sender_loss_rate_in_packets(), 0.05);
+ if (GetQuicReloadableFlag(quic_fix_pacing_sender_bursts)) {
+ EXPECT_EQ(sender_loss_rate_in_packets(), 0);
+ } else {
+ EXPECT_LE(sender_loss_rate_in_packets(), 0.05);
+ }
// The margin here is high, because both link level aggregation and ack
// decimation can greatly increase smoothed rtt.
EXPECT_GE(params.RTT() * 5, rtt_stats()->smoothed_rtt());
@@ -558,6 +562,9 @@
[this]() { return sender_endpoint_.bytes_to_transfer() == 0; },
QuicTime::Delta::FromSeconds(50));
EXPECT_TRUE(simulator_result);
+ // Ensure the full bandwidth is discovered.
+ EXPECT_APPROX_EQ(params.test_link.bandwidth,
+ sender_->ExportDebugState().bandwidth_hi, 0.02f);
}
// Test the number of losses incurred by the startup phase in a situation when
diff --git a/quic/core/congestion_control/pacing_sender.cc b/quic/core/congestion_control/pacing_sender.cc
index a43601e..265a422 100644
--- a/quic/core/congestion_control/pacing_sender.cc
+++ b/quic/core/congestion_control/pacing_sender.cc
@@ -102,6 +102,12 @@
// is about 10ms of queueing.
lumpy_tokens_ = 1u;
}
+ if (GetQuicReloadableFlag(quic_fix_pacing_sender_bursts) &&
+ (bytes_in_flight + bytes) >= sender_->GetCongestionWindow()) {
+ QUIC_RELOADABLE_FLAG_COUNT(quic_fix_pacing_sender_bursts);
+ // Don't add lumpy_tokens if the congestion controller is CWND limited.
+ lumpy_tokens_ = 1u;
+ }
}
--lumpy_tokens_;
if (pacing_limited_) {
diff --git a/quic/core/congestion_control/pacing_sender_test.cc b/quic/core/congestion_control/pacing_sender_test.cc
index 9c28e9f..ec3119d 100644
--- a/quic/core/congestion_control/pacing_sender_test.cc
+++ b/quic/core/congestion_control/pacing_sender_test.cc
@@ -7,6 +7,7 @@
#include <memory>
#include <utility>
+#include "quic/core/quic_constants.h"
#include "quic/core/quic_packets.h"
#include "quic/platform/api/quic_flag_utils.h"
#include "quic/platform/api/quic_flags.h"
@@ -96,7 +97,6 @@
OnPacketSent(clock_.Now(), prior_in_flight, packet_number_,
kMaxOutgoingPacketSize, retransmittable_data));
EXPECT_CALL(*mock_sender_, GetCongestionWindow())
- .Times(AtMost(1))
.WillRepeatedly(Return(cwnd * kDefaultTCPMSS));
EXPECT_CALL(*mock_sender_,
CanSend(prior_in_flight + kMaxOutgoingPacketSize))
@@ -459,7 +459,7 @@
}
TEST_F(PacingSenderTest, NoLumpyPacingForLowBandwidthFlows) {
- // Set lumpy size to be 3, and cwnd faction to 0.5
+ // Set lumpy size to be 3, and cwnd fraction to 0.5
SetQuicFlag(FLAGS_quic_lumpy_pacing_size, 3);
SetQuicFlag(FLAGS_quic_lumpy_pacing_cwnd_fraction, 0.5f);
@@ -484,6 +484,43 @@
}
}
+// Regression test for b/184471302 to ensure that ACKs received back-to-back
+// don't cause bursts in sending.
+TEST_F(PacingSenderTest, NoBurstsForLumpyPacingWithAckAggregation) {
+ // Configure pacing rate of 1 packet per millisecond.
+ QuicTime::Delta inter_packet_delay = QuicTime::Delta::FromMilliseconds(1);
+ InitPacingRate(kInitialBurstPackets,
+ QuicBandwidth::FromBytesAndTimeDelta(kMaxOutgoingPacketSize,
+ inter_packet_delay));
+ UpdateRtt();
+
+ // Send kInitialBurstPackets packets, and verify that they are not paced.
+ for (int i = 0; i < kInitialBurstPackets; ++i) {
+ CheckPacketIsSentImmediately();
+ }
+ // The last packet of the burst causes the sender to be CWND limited.
+ CheckPacketIsSentImmediately(HAS_RETRANSMITTABLE_DATA,
+ 10 * kMaxOutgoingPacketSize, false, 10);
+
+ if (GetQuicReloadableFlag(quic_fix_pacing_sender_bursts)) {
+ // The last sent packet made the connection CWND limited, so no lumpy tokens
+ // should be available.
+ EXPECT_EQ(0u, pacing_sender_->lumpy_tokens());
+ CheckPacketIsSentImmediately(HAS_RETRANSMITTABLE_DATA,
+ 10 * kMaxOutgoingPacketSize, false, 10);
+ EXPECT_EQ(0u, pacing_sender_->lumpy_tokens());
+ CheckPacketIsDelayed(2 * inter_packet_delay);
+ } else {
+ EXPECT_EQ(1u, pacing_sender_->lumpy_tokens());
+ // Repeatedly send single packets to make the sender CWND limited and
+ // observe that there's no pacing without the fix.
+ for (int i = 0; i < 10; ++i) {
+ CheckPacketIsSentImmediately(HAS_RETRANSMITTABLE_DATA,
+ 10 * kMaxOutgoingPacketSize, false, 10);
+ }
+ }
+}
+
TEST_F(PacingSenderTest, IdealNextPacketSendTimeWithLumpyPacing) {
// Set lumpy size to be 3, and cwnd faction to 0.5
SetQuicFlag(FLAGS_quic_lumpy_pacing_size, 3);
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 6c6df06..0afbf73 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -131,6 +131,8 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_h3_datagram, false)
// When true, defaults to BBR congestion control instead of Cubic.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_default_to_bbr, false)
+// When true, prevents QUIC\'s PacingSender from generating bursts when the congestion controller is CWND limited and not pacing limited.
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_pacing_sender_bursts, false)
// When true, set the initial congestion control window from connection options in QuicSentPacketManager rather than TcpCubicSenderBytes.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_unified_iw_options, false)
