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; }