gfe-relnote: (n/a) Deprecate --gfe2_reloadable_flag_quic_bw_sampler_remove_packets_once_per_congestion_event2.
PiperOrigin-RevId: 296882742
Change-Id: I9d85c417654c90fb2ce99c850b9f57bae93674d3
diff --git a/quic/core/congestion_control/bandwidth_sampler.cc b/quic/core/congestion_control/bandwidth_sampler.cc
index 1a56e62..dc8509f 100644
--- a/quic/core/congestion_control/bandwidth_sampler.cc
+++ b/quic/core/congestion_control/bandwidth_sampler.cc
@@ -85,10 +85,6 @@
max_ack_height_tracker_(max_height_tracker_window_length),
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);
- }
if (one_bw_sample_per_ack_event_) {
QUIC_RELOADABLE_FLAG_COUNT(quic_one_bw_sample_per_ack_event2);
}
@@ -283,9 +279,6 @@
}
BandwidthSample sample =
OnPacketAcknowledgedInner(ack_time, packet_number, *sent_packet_pointer);
- if (!remove_packets_once_per_congestion_event_) {
- connection_state_map_.Remove(packet_number);
- }
return sample;
}
@@ -403,20 +396,13 @@
// QUIC_BUG when removal fails.
SendTimeState send_time_state;
- if (remove_packets_once_per_congestion_event_) {
- total_bytes_lost_ += bytes_lost;
- ConnectionStateOnSentPacket* sent_packet_pointer =
- connection_state_map_.GetEntry(packet_number);
- if (sent_packet_pointer != nullptr) {
- SentPacketToSendTimeState(*sent_packet_pointer, &send_time_state);
- }
- } else {
- send_time_state.is_valid = connection_state_map_.Remove(
- packet_number, [&](const ConnectionStateOnSentPacket& sent_packet) {
- total_bytes_lost_ += sent_packet.size;
- SentPacketToSendTimeState(sent_packet, &send_time_state);
- });
+ total_bytes_lost_ += bytes_lost;
+ ConnectionStateOnSentPacket* sent_packet_pointer =
+ connection_state_map_.GetEntry(packet_number);
+ if (sent_packet_pointer != nullptr) {
+ SentPacketToSendTimeState(*sent_packet_pointer, &send_time_state);
}
+
return send_time_state;
}
@@ -438,20 +424,7 @@
// QuicSentPacketManager::RetransmitCryptoPackets retransmits a crypto packet,
// the packet is removed from QuicUnackedPacketMap's inflight, but is not
// marked as acked or lost in the BandwidthSampler.
- if (remove_packets_once_per_congestion_event_) {
- connection_state_map_.RemoveUpTo(least_unacked);
- return;
- }
- while (!connection_state_map_.IsEmpty() &&
- connection_state_map_.first_packet() < least_unacked) {
- connection_state_map_.Remove(
- connection_state_map_.first_packet(),
- [&](const ConnectionStateOnSentPacket& sent_packet) {
- // Obsoleted packets as either acked or lost but the sampler doesn't
- // know. We count them as acked here, since most packets are acked.
- total_bytes_acked_ += sent_packet.size;
- });
- }
+ connection_state_map_.RemoveUpTo(least_unacked);
}
QuicByteCount BandwidthSampler::total_bytes_sent() const {
diff --git a/quic/core/congestion_control/bandwidth_sampler.h b/quic/core/congestion_control/bandwidth_sampler.h
index 210a219..6f27ddd 100644
--- a/quic/core/congestion_control/bandwidth_sampler.h
+++ b/quic/core/congestion_control/bandwidth_sampler.h
@@ -365,10 +365,6 @@
max_ack_height_tracker_.Reset(new_height, new_time);
}
- bool remove_packets_once_per_congestion_event() const {
- return remove_packets_once_per_congestion_event_;
- }
-
bool one_bw_sample_per_ack_event() const {
return one_bw_sample_per_ack_event_;
}
@@ -471,9 +467,7 @@
sampler.total_bytes_sent_,
sampler.total_bytes_acked_,
sampler.total_bytes_lost_,
- sampler.remove_packets_once_per_congestion_event()
- ? bytes_in_flight
- : 0) {}
+ bytes_in_flight) {}
// Default constructor. Required to put this structure into
// PacketNumberIndexedQueue.
@@ -563,14 +557,8 @@
MaxAckHeightTracker max_ack_height_tracker_;
QuicByteCount total_bytes_acked_after_last_ack_event_;
- // Latched value of quic_bw_sampler_remove_packets_once_per_congestion_event2.
- const bool remove_packets_once_per_congestion_event_ = GetQuicReloadableFlag(
- 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_event2.
+ // Latched value of 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_event2);
// True if --quic_avoid_overestimate_bandwidth_with_aggregation=true and
diff --git a/quic/core/congestion_control/bandwidth_sampler_test.cc b/quic/core/congestion_control/bandwidth_sampler_test.cc
index 2529ba1..c523654 100644
--- a/quic/core/congestion_control/bandwidth_sampler_test.cc
+++ b/quic/core/congestion_control/bandwidth_sampler_test.cc
@@ -203,9 +203,8 @@
QuicBandwidth current_sample = AckPacket(i);
EXPECT_EQ(expected_bandwidth, current_sample);
}
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(25));
- }
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(25));
+
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
EXPECT_EQ(0u, bytes_in_flight_);
}
@@ -263,15 +262,13 @@
EXPECT_EQ(PacketsToBytes(1), send_time_state.total_bytes_acked);
EXPECT_EQ(PacketsToBytes(2), send_time_state.total_bytes_lost);
}
- if (sampler_.remove_packets_once_per_congestion_event()) {
- // This equation works because there is no neutered bytes.
- EXPECT_EQ(send_time_state.total_bytes_sent -
- send_time_state.total_bytes_acked -
- send_time_state.total_bytes_lost,
- send_time_state.bytes_in_flight);
- } else {
- EXPECT_EQ(0u, send_time_state.bytes_in_flight);
- }
+
+ // This equation works because there is no neutered bytes.
+ EXPECT_EQ(send_time_state.total_bytes_sent -
+ send_time_state.total_bytes_acked -
+ send_time_state.total_bytes_lost,
+ send_time_state.bytes_in_flight);
+
clock_.AdvanceTime(time_between_packets);
}
}
@@ -293,9 +290,8 @@
EXPECT_EQ(expected_bandwidth, last_bandwidth) << "i is " << i;
clock_.AdvanceTime(time_between_packets);
}
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
- }
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
+
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
EXPECT_EQ(0u, bytes_in_flight_);
}
@@ -336,9 +332,8 @@
}
clock_.AdvanceTime(time_between_packets);
}
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
- }
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
+
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
EXPECT_EQ(0u, bytes_in_flight_);
}
@@ -386,10 +381,8 @@
}
clock_.AdvanceTime(time_between_packets);
}
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
- }
// Since only congestion controlled packets are entered into the map, it has
// to be empty at this point.
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
@@ -418,9 +411,9 @@
clock_.AdvanceTime(ridiculously_small_time_delta);
}
EXPECT_EQ(expected_bandwidth, last_bandwidth);
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
- }
+
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(41));
+
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
EXPECT_EQ(0u, bytes_in_flight_);
}
@@ -450,9 +443,8 @@
EXPECT_EQ(expected_bandwidth, last_bandwidth);
clock_.AdvanceTime(time_between_packets);
}
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(61));
- }
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(61));
+
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
EXPECT_EQ(0u, bytes_in_flight_);
}
@@ -502,10 +494,8 @@
EXPECT_EQ(expected_bandwidth, last_bandwidth);
clock_.AdvanceTime(time_between_packets);
}
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(81));
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(81));
- }
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
EXPECT_EQ(0u, bytes_in_flight_);
}
@@ -559,14 +549,13 @@
sampler_.RemoveObsoletePackets(QuicPacketNumber(4));
EXPECT_EQ(2u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
sampler_.OnPacketLost(QuicPacketNumber(4), kRegularPacketSize);
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(5));
- }
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(5));
+
EXPECT_EQ(1u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
AckPacket(5);
- if (sampler_.remove_packets_once_per_congestion_event()) {
- sampler_.RemoveObsoletePackets(QuicPacketNumber(6));
- }
+
+ sampler_.RemoveObsoletePackets(QuicPacketNumber(6));
+
EXPECT_EQ(0u, BandwidthSamplerPeer::GetNumberOfTrackedPackets(sampler_));
}
diff --git a/quic/core/congestion_control/bbr2_misc.cc b/quic/core/congestion_control/bbr2_misc.cc
index 352e7e6..6c3ea73 100644
--- a/quic/core/congestion_control/bbr2_misc.cc
+++ b/quic/core/congestion_control/bbr2_misc.cc
@@ -164,26 +164,21 @@
}
}
- if (!bandwidth_sampler_.remove_packets_once_per_congestion_event()) {
- congestion_event->bytes_in_flight = bytes_in_flight();
- }
-
congestion_event->bytes_acked = total_bytes_acked() - prior_bytes_acked;
congestion_event->bytes_lost = total_bytes_lost() - prior_bytes_lost;
- if (bandwidth_sampler_.remove_packets_once_per_congestion_event()) {
- if (congestion_event->prior_bytes_in_flight >=
- congestion_event->bytes_acked + congestion_event->bytes_lost) {
- congestion_event->bytes_in_flight =
- congestion_event->prior_bytes_in_flight -
- congestion_event->bytes_acked - congestion_event->bytes_lost;
- } else {
- QUIC_LOG_FIRST_N(ERROR, 1)
- << "prior_bytes_in_flight:" << congestion_event->prior_bytes_in_flight
- << " is smaller than the sum of bytes_acked:"
- << congestion_event->bytes_acked
- << " and bytes_lost:" << congestion_event->bytes_lost;
- congestion_event->bytes_in_flight = 0;
- }
+
+ if (congestion_event->prior_bytes_in_flight >=
+ congestion_event->bytes_acked + congestion_event->bytes_lost) {
+ congestion_event->bytes_in_flight =
+ congestion_event->prior_bytes_in_flight -
+ congestion_event->bytes_acked - congestion_event->bytes_lost;
+ } else {
+ QUIC_LOG_FIRST_N(ERROR, 1)
+ << "prior_bytes_in_flight:" << congestion_event->prior_bytes_in_flight
+ << " is smaller than the sum of bytes_acked:"
+ << congestion_event->bytes_acked
+ << " and bytes_lost:" << congestion_event->bytes_lost;
+ congestion_event->bytes_in_flight = 0;
}
if (congestion_event->bytes_lost > 0) {
diff --git a/quic/core/congestion_control/bbr2_misc.h b/quic/core/congestion_control/bbr2_misc.h
index 191790b..500226e 100644
--- a/quic/core/congestion_control/bbr2_misc.h
+++ b/quic/core/congestion_control/bbr2_misc.h
@@ -404,11 +404,6 @@
return bandwidth_sampler_.total_bytes_sent();
}
- QuicByteCount bytes_in_flight() const {
- DCHECK(!bandwidth_sampler_.remove_packets_once_per_congestion_event());
- return total_bytes_sent() - total_bytes_acked() - total_bytes_lost();
- }
-
int64_t loss_events_in_round() const { return loss_events_in_round_; }
bool always_count_loss_events() const { return always_count_loss_events_; }
diff --git a/quic/core/congestion_control/bbr_sender_test.cc b/quic/core/congestion_control/bbr_sender_test.cc
index 2b262c9..55ac77c 100644
--- a/quic/core/congestion_control/bbr_sender_test.cc
+++ b/quic/core/congestion_control/bbr_sender_test.cc
@@ -966,9 +966,7 @@
// Test exiting STARTUP earlier upon loss due to the LRTT connection option.
TEST_F(BbrSenderTest, SimpleTransferLRTTStartup) {
- if (!GetQuicReloadableFlag(
- quic_bw_sampler_remove_packets_once_per_congestion_event2) ||
- !GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event2) ||
+ if (!GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event2) ||
!GetQuicReloadableFlag(quic_bbr_loss_based_startup_exit)) {
return;
}
@@ -1000,9 +998,7 @@
// Test exiting STARTUP earlier upon loss due to the LRTT connection option.
TEST_F(BbrSenderTest, SimpleTransferLRTTStartupSmallBuffer) {
- if (!GetQuicReloadableFlag(
- quic_bw_sampler_remove_packets_once_per_congestion_event2) ||
- !GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event2) ||
+ if (!GetQuicReloadableFlag(quic_one_bw_sample_per_ack_event2) ||
!GetQuicReloadableFlag(quic_bbr_loss_based_startup_exit)) {
return;
}
