gfe-relnote: Deprecate gfe2_reloadable_flag_quic_detect_spurious_loss. PiperOrigin-RevId: 276694630 Change-Id: I76f4f8cb9098fdb63cb5f9e3d68c11e97546c1ab
diff --git a/quic/core/congestion_control/general_loss_algorithm.cc b/quic/core/congestion_control/general_loss_algorithm.cc index 3502031..5f7115a 100644 --- a/quic/core/congestion_control/general_loss_algorithm.cc +++ b/quic/core/congestion_control/general_loss_algorithm.cc
@@ -39,7 +39,6 @@ void GeneralLossAlgorithm::SetLossDetectionType(LossDetectionType loss_type) { loss_detection_timeout_ = QuicTime::Zero(); - largest_sent_on_spurious_retransmit_.Clear(); loss_type_ = loss_type; if (loss_type == kAdaptiveTime) { reordering_shift_ = kDefaultAdaptiveLossDelayShift; @@ -185,36 +184,6 @@ return loss_detection_timeout_; } -void GeneralLossAlgorithm::SpuriousRetransmitDetected( - const QuicUnackedPacketMap& unacked_packets, - QuicTime time, - const RttStats& rtt_stats, - QuicPacketNumber spurious_retransmission) { - if (loss_type_ != kAdaptiveTime || reordering_shift_ == 0) { - return; - } - // Calculate the extra time needed so this wouldn't have been declared lost. - // Extra time needed is based on how long it's been since the spurious - // retransmission was sent, because the SRTT and latest RTT may have changed. - QuicTime::Delta extra_time_needed = - time - - unacked_packets.GetTransmissionInfo(spurious_retransmission).sent_time; - // Increase the reordering fraction until enough time would be allowed. - QuicTime::Delta max_rtt = - std::max(rtt_stats.previous_srtt(), rtt_stats.latest_rtt()); - - if (largest_sent_on_spurious_retransmit_.IsInitialized() && - spurious_retransmission <= largest_sent_on_spurious_retransmit_) { - return; - } - largest_sent_on_spurious_retransmit_ = unacked_packets.largest_sent_packet(); - QuicTime::Delta proposed_extra_time(QuicTime::Delta::Zero()); - do { - proposed_extra_time = max_rtt >> reordering_shift_; - --reordering_shift_; - } while (proposed_extra_time < extra_time_needed && reordering_shift_ > 0); -} - void GeneralLossAlgorithm::SpuriousLossDetected( const QuicUnackedPacketMap& unacked_packets, const RttStats& rtt_stats,
diff --git a/quic/core/congestion_control/general_loss_algorithm.h b/quic/core/congestion_control/general_loss_algorithm.h index c9da837..d501cc6 100644 --- a/quic/core/congestion_control/general_loss_algorithm.h +++ b/quic/core/congestion_control/general_loss_algorithm.h
@@ -47,13 +47,6 @@ // Returns a non-zero value when the early retransmit timer is active. QuicTime GetLossTimeout() const override; - // Increases the loss detection threshold for time loss detection. - void SpuriousRetransmitDetected( - const QuicUnackedPacketMap& unacked_packets, - QuicTime time, - const RttStats& rtt_stats, - QuicPacketNumber spurious_retransmission) override; - // Called to increases time and/or packet threshold. void SpuriousLossDetected(const QuicUnackedPacketMap& unacked_packets, const RttStats& rtt_stats, @@ -79,9 +72,6 @@ private: QuicTime loss_detection_timeout_; - // Largest sent packet when a spurious retransmit is detected. - // Prevents increasing the reordering threshold multiple times per epoch. - QuicPacketNumber largest_sent_on_spurious_retransmit_; LossDetectionType loss_type_; // Fraction of a max(SRTT, latest_rtt) to permit reordering before declaring // loss. Fraction calculated by shifting max(SRTT, latest_rtt) to the right
diff --git a/quic/core/congestion_control/general_loss_algorithm_test.cc b/quic/core/congestion_control/general_loss_algorithm_test.cc index f1ba35a..0c41f65 100644 --- a/quic/core/congestion_control/general_loss_algorithm_test.cc +++ b/quic/core/congestion_control/general_loss_algorithm_test.cc
@@ -514,21 +514,9 @@ // Advance the time 1/4 RTT and indicate the loss was spurious. // The new threshold should be 1/2 RTT. clock_.AdvanceTime(rtt_stats_.smoothed_rtt() * (1.0f / 4)); - if (GetQuicReloadableFlag(quic_detect_spurious_loss)) { - loss_algorithm_.SpuriousLossDetected(unacked_packets_, rtt_stats_, - clock_.Now(), QuicPacketNumber(1), - QuicPacketNumber(2)); - EXPECT_EQ(1, loss_algorithm_.reordering_shift()); - return; - } - loss_algorithm_.SpuriousRetransmitDetected(unacked_packets_, clock_.Now(), - rtt_stats_, QuicPacketNumber(11)); - EXPECT_EQ(1, loss_algorithm_.reordering_shift()); - - // Detect another spurious retransmit and ensure the threshold doesn't - // increase again. - loss_algorithm_.SpuriousRetransmitDetected(unacked_packets_, clock_.Now(), - rtt_stats_, QuicPacketNumber(12)); + loss_algorithm_.SpuriousLossDetected(unacked_packets_, rtt_stats_, + clock_.Now(), QuicPacketNumber(1), + QuicPacketNumber(2)); EXPECT_EQ(1, loss_algorithm_.reordering_shift()); }
diff --git a/quic/core/congestion_control/loss_detection_interface.h b/quic/core/congestion_control/loss_detection_interface.h index f686785..af9c196 100644 --- a/quic/core/congestion_control/loss_detection_interface.h +++ b/quic/core/congestion_control/loss_detection_interface.h
@@ -35,16 +35,6 @@ // Returns QuicTime::Zero if no alarm needs to be set. virtual QuicTime GetLossTimeout() const = 0; - // Called when a |spurious_retransmission| is detected. The original - // transmission must have been caused by DetectLosses. - // TODO(fayang): Remove this method when deprecating - // quic_detect_spurious_loss. - virtual void SpuriousRetransmitDetected( - const QuicUnackedPacketMap& unacked_packets, - QuicTime time, - const RttStats& rtt_stats, - QuicPacketNumber spurious_retransmission) = 0; - // Called when |packet_number| was detected lost but gets acked later. virtual void SpuriousLossDetected( const QuicUnackedPacketMap& unacked_packets,
diff --git a/quic/core/congestion_control/uber_loss_algorithm.cc b/quic/core/congestion_control/uber_loss_algorithm.cc index 3b669e2..1aa10ff 100644 --- a/quic/core/congestion_control/uber_loss_algorithm.cc +++ b/quic/core/congestion_control/uber_loss_algorithm.cc
@@ -70,17 +70,6 @@ return loss_timeout; } -void UberLossAlgorithm::SpuriousRetransmitDetected( - const QuicUnackedPacketMap& unacked_packets, - QuicTime time, - const RttStats& rtt_stats, - QuicPacketNumber spurious_retransmission) { - general_loss_algorithms_[unacked_packets.GetPacketNumberSpace( - spurious_retransmission)] - .SpuriousRetransmitDetected(unacked_packets, time, rtt_stats, - spurious_retransmission); -} - void UberLossAlgorithm::SpuriousLossDetected( const QuicUnackedPacketMap& unacked_packets, const RttStats& rtt_stats,
diff --git a/quic/core/congestion_control/uber_loss_algorithm.h b/quic/core/congestion_control/uber_loss_algorithm.h index 0d2b788..a97fe08 100644 --- a/quic/core/congestion_control/uber_loss_algorithm.h +++ b/quic/core/congestion_control/uber_loss_algorithm.h
@@ -41,13 +41,6 @@ // Returns the earliest time the early retransmit timer should be active. QuicTime GetLossTimeout() const override; - // Increases the loss detection threshold for time loss detection. - void SpuriousRetransmitDetected( - const QuicUnackedPacketMap& unacked_packets, - QuicTime time, - const RttStats& rtt_stats, - QuicPacketNumber spurious_retransmission) override; - // Called to increases time or packet threshold. void SpuriousLossDetected(const QuicUnackedPacketMap& unacked_packets, const RttStats& rtt_stats,
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 13ac894..852f9d2 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -103,7 +103,6 @@ consecutive_pto_count_(0), handshake_mode_disabled_(false), forward_secure_packet_acked_(false), - detect_spurious_losses_(GetQuicReloadableFlag(quic_detect_spurious_loss)), neuter_handshake_packets_once_( GetQuicReloadableFlag(quic_neuter_handshake_packets_once)) { SetSendAlgorithm(congestion_control_type); @@ -253,18 +252,16 @@ uber_loss_algorithm_.SetLossDetectionType(kIetfLossDetection); uber_loss_algorithm_.SetReorderingShift(kDefaultLossDelayShift); } - if (GetQuicReloadableFlag(quic_detect_spurious_loss)) { - if (config.HasClientRequestedIndependentOption(kILD2, perspective)) { - QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_ietf_loss_detection, 3, 4); - uber_loss_algorithm_.SetLossDetectionType(kIetfLossDetection); - uber_loss_algorithm_.EnableAdaptiveReorderingThreshold(); - } - if (config.HasClientRequestedIndependentOption(kILD3, perspective)) { - QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_ietf_loss_detection, 4, 4); - uber_loss_algorithm_.SetLossDetectionType(kIetfLossDetection); - uber_loss_algorithm_.SetReorderingShift(kDefaultLossDelayShift); - uber_loss_algorithm_.EnableAdaptiveReorderingThreshold(); - } + if (config.HasClientRequestedIndependentOption(kILD2, perspective)) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_ietf_loss_detection, 3, 4); + uber_loss_algorithm_.SetLossDetectionType(kIetfLossDetection); + uber_loss_algorithm_.EnableAdaptiveReorderingThreshold(); + } + if (config.HasClientRequestedIndependentOption(kILD3, perspective)) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_ietf_loss_detection, 4, 4); + uber_loss_algorithm_.SetLossDetectionType(kIetfLossDetection); + uber_loss_algorithm_.SetReorderingShift(kDefaultLossDelayShift); + uber_loss_algorithm_.EnableAdaptiveReorderingThreshold(); } } if (config.HasClientSentConnectionOption(kCONH, perspective)) { @@ -515,18 +512,6 @@ } } -void QuicSentPacketManager::RecordSpuriousRetransmissions( - const QuicTransmissionInfo& info, - QuicPacketNumber acked_packet_number) { - RecordOneSpuriousRetransmission(info); - if (!detect_spurious_losses_ && - info.transmission_type == LOSS_RETRANSMISSION) { - // Only inform the loss detection of spurious retransmits it caused. - loss_algorithm_->SpuriousRetransmitDetected( - unacked_packets_, clock_->Now(), rtt_stats_, acked_packet_number); - } -} - void QuicSentPacketManager::MarkPacketHandled(QuicPacketNumber packet_number, QuicTransmissionInfo* info, QuicTime ack_receive_time, @@ -547,13 +532,12 @@ QUIC_DVLOG(1) << "Detect spurious retransmitted packet " << packet_number << " transmission type: " << TransmissionTypeToString(info->transmission_type); - RecordSpuriousRetransmissions(*info, packet_number); + RecordOneSpuriousRetransmission(*info); } } - if (detect_spurious_losses_ && info->state == LOST) { + if (info->state == LOST) { // Record as a spurious loss as a packet previously declared lost gets // acked. - QUIC_RELOADABLE_FLAG_COUNT(quic_detect_spurious_loss); const PacketNumberSpace packet_number_space = unacked_packets_.GetPacketNumberSpace(info->encryption_level); const QuicPacketNumber previous_largest_acked =
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index a517dec..e6a5b6f 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -491,11 +491,6 @@ // this function. void RecordOneSpuriousRetransmission(const QuicTransmissionInfo& info); - // Notify observers about spurious retransmits of packet with - // QuicTransmissionInfo |info|. - void RecordSpuriousRetransmissions(const QuicTransmissionInfo& info, - QuicPacketNumber acked_packet_number); - // Sets the initial RTT of the connection. void SetInitialRtt(QuicTime::Delta rtt); @@ -626,9 +621,6 @@ // ENCRYPTION_FORWARD_SECURE packet. bool forward_secure_packet_acked_; - // Latched value of quic_detect_spurious_loss. - const bool detect_spurious_losses_; - // Latched value of quic_neuter_handshake_packets_once. const bool neuter_handshake_packets_once_; };
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 7872f74..e86d950 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -663,14 +663,9 @@ uint64_t acked[] = {3}; ExpectAcksAndLosses(false, acked, QUIC_ARRAYSIZE(acked), nullptr, 0); EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _)); - if (GetQuicReloadableFlag(quic_detect_spurious_loss)) { - EXPECT_CALL(*loss_algorithm, - SpuriousLossDetected(_, _, _, QuicPacketNumber(3), - QuicPacketNumber(4))); - } else { - EXPECT_CALL(*loss_algorithm, - SpuriousRetransmitDetected(_, _, _, QuicPacketNumber(5))); - } + EXPECT_CALL(*loss_algorithm, + SpuriousLossDetected(_, _, _, QuicPacketNumber(3), + QuicPacketNumber(4))); manager_.OnAckFrameStart(QuicPacketNumber(4), QuicTime::Delta::Infinite(), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); @@ -1810,7 +1805,6 @@ TEST_F(QuicSentPacketManagerTest, NegotiateIetfLossDetectionAdaptiveReorderingThreshold) { SetQuicReloadableFlag(quic_enable_ietf_loss_detection, true); - SetQuicReloadableFlag(quic_detect_spurious_loss, true); EXPECT_EQ(kNack, QuicSentPacketManagerPeer::GetLossAlgorithm(&manager_) ->GetLossDetectionType()); EXPECT_FALSE( @@ -1835,7 +1829,6 @@ TEST_F(QuicSentPacketManagerTest, NegotiateIetfLossDetectionAdaptiveReorderingThreshold2) { SetQuicReloadableFlag(quic_enable_ietf_loss_detection, true); - SetQuicReloadableFlag(quic_detect_spurious_loss, true); EXPECT_EQ(kNack, QuicSentPacketManagerPeer::GetLossAlgorithm(&manager_) ->GetLossDetectionType()); EXPECT_FALSE(
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h index 9f8f23c..e39b416 100644 --- a/quic/test_tools/quic_test_utils.h +++ b/quic/test_tools/quic_test_utils.h
@@ -966,11 +966,6 @@ const AckedPacketVector& packets_acked, LostPacketVector* packets_lost)); MOCK_CONST_METHOD0(GetLossTimeout, QuicTime()); - MOCK_METHOD4(SpuriousRetransmitDetected, - void(const QuicUnackedPacketMap&, - QuicTime, - const RttStats&, - QuicPacketNumber)); MOCK_METHOD5(SpuriousLossDetected, void(const QuicUnackedPacketMap&, const RttStats&,