gfe-relnote: In QUIC bbr sender, fix calculation of bytes_lost when gfe2_reloadable_flag_quic_one_bw_sample_per_ack_event is on. Protected by v2 flag gfe2_reloadable_flag_quic_one_bw_sample_per_ack_event2.
Also added a unit test to ensure bytes_lost is used to update recover window.
PiperOrigin-RevId: 289101409
Change-Id: I17ff916303af25a3967fb0559bbc50ec4ace4122
diff --git a/quic/core/congestion_control/bandwidth_sampler.cc b/quic/core/congestion_control/bandwidth_sampler.cc
index 21705a5..d4eeabe 100644
--- a/quic/core/congestion_control/bandwidth_sampler.cc
+++ b/quic/core/congestion_control/bandwidth_sampler.cc
@@ -87,7 +87,7 @@
quic_bw_sampler_remove_packets_once_per_congestion_event2);
}
if (one_bw_sample_per_ack_event_) {
- QUIC_RELOADABLE_FLAG_COUNT(quic_one_bw_sample_per_ack_event);
+ QUIC_RELOADABLE_FLAG_COUNT(quic_one_bw_sample_per_ack_event2);
}
}
diff --git a/quic/core/congestion_control/bandwidth_sampler.h b/quic/core/congestion_control/bandwidth_sampler.h
index d93af36..df63fa5 100644
--- a/quic/core/congestion_control/bandwidth_sampler.h
+++ b/quic/core/congestion_control/bandwidth_sampler.h
@@ -481,10 +481,10 @@
quic_bw_sampler_remove_packets_once_per_congestion_event2);
// Latched value of quic_bw_sampler_remove_packets_once_per_congestion_event2
- // and quic_one_bw_sample_per_ack_event.
+ // and quic_one_bw_sample_per_ack_event2.
const bool one_bw_sample_per_ack_event_ =
remove_packets_once_per_congestion_event_ &&
- GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event);
+ GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event2);
};
} // namespace quic
diff --git a/quic/core/congestion_control/bbr_sender.cc b/quic/core/congestion_control/bbr_sender.cc
index f8d22db..f80d4ce 100644
--- a/quic/core/congestion_control/bbr_sender.cc
+++ b/quic/core/congestion_control/bbr_sender.cc
@@ -443,8 +443,8 @@
if (!sample.sample_rtt.IsInfinite()) {
min_rtt_expired = MaybeUpdateMinRtt(event_time, sample.sample_rtt);
}
+ bytes_lost = sampler_.total_bytes_lost() - total_bytes_lost_before;
if (mode_ == STARTUP) {
- bytes_lost = sampler_.total_bytes_lost() - total_bytes_lost_before;
if (stats_) {
stats_->slowstart_packets_lost += lost_packets.size();
stats_->slowstart_bytes_lost += bytes_lost;
diff --git a/quic/core/congestion_control/bbr_sender_test.cc b/quic/core/congestion_control/bbr_sender_test.cc
index ddb1077..2ff1f8a 100644
--- a/quic/core/congestion_control/bbr_sender_test.cc
+++ b/quic/core/congestion_control/bbr_sender_test.cc
@@ -11,6 +11,7 @@
#include "net/third_party/quiche/src/quic/core/congestion_control/rtt_stats.h"
#include "net/third_party/quiche/src/quic/core/quic_packets.h"
+#include "net/third_party/quiche/src/quic/core/quic_types.h"
#include "net/third_party/quiche/src/quic/core/quic_utils.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_flags.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_logging.h"
@@ -377,6 +378,54 @@
EXPECT_FALSE(sender_->ExportDebugState().last_sample_is_app_limited);
}
+TEST_F(BbrSenderTest, RemoveBytesLostInRecovery) {
+ SetQuicReloadableFlag(quic_bbr_one_mss_conservation, false);
+ // Disable Ack Decimation on the receiver, because it can increase srtt.
+ QuicConnectionPeer::SetAckMode(receiver_.connection(), AckMode::TCP_ACKING);
+ CreateDefaultSetup();
+
+ DriveOutOfStartup();
+
+ // Drop a packet to enter recovery.
+ receiver_.DropNextIncomingPacket();
+ ASSERT_TRUE(
+ simulator_.RunUntilOrTimeout([this]() { return sender_->InRecovery(); },
+ QuicTime::Delta::FromSeconds(30)));
+
+ QuicUnackedPacketMap* unacked_packets =
+ QuicSentPacketManagerPeer::GetUnackedPacketMap(
+ QuicConnectionPeer::GetSentPacketManager(bbr_sender_.connection()));
+ QuicPacketNumber largest_sent =
+ bbr_sender_.connection()->sent_packet_manager().GetLargestSentPacket();
+ // least_inflight is the smallest inflight packet.
+ QuicPacketNumber least_inflight =
+ bbr_sender_.connection()->sent_packet_manager().GetLeastUnacked();
+ while (!unacked_packets->GetTransmissionInfo(least_inflight).in_flight) {
+ ASSERT_LE(least_inflight, largest_sent);
+ least_inflight++;
+ }
+ QuicPacketLength least_inflight_packet_size =
+ unacked_packets->GetTransmissionInfo(least_inflight).bytes_sent;
+ QuicByteCount prior_recovery_window =
+ sender_->ExportDebugState().recovery_window;
+ QuicByteCount prior_inflight = unacked_packets->bytes_in_flight();
+ QUIC_LOG(INFO) << "Recovery window:" << prior_recovery_window
+ << ", least_inflight_packet_size:"
+ << least_inflight_packet_size
+ << ", bytes_in_flight:" << prior_inflight;
+ ASSERT_GT(prior_recovery_window, least_inflight_packet_size);
+
+ // Lose the least inflight packet and expect the recovery window to drop.
+ unacked_packets->RemoveFromInFlight(least_inflight);
+ LostPacketVector lost_packets;
+ lost_packets.emplace_back(least_inflight, least_inflight_packet_size);
+ sender_->OnCongestionEvent(false, prior_inflight, clock_->Now(), {},
+ lost_packets);
+ EXPECT_EQ(sender_->ExportDebugState().recovery_window,
+ prior_inflight - least_inflight_packet_size);
+ EXPECT_LT(sender_->ExportDebugState().recovery_window, prior_recovery_window);
+}
+
// Test a simple long data transfer with 2 rtts of aggregation.
TEST_F(BbrSenderTest, SimpleTransfer2RTTAggregationBytes) {
CreateDefaultSetup();
