In quic loss detection tuner, do not give feedback if a connection 1) has no packets lost, 2) would still have no packets lost if the reordering_shift increased by one. protected by existing flag --gfe2_reloadable_flag_quic_enable_loss_detection_experiment_at_gfe. PiperOrigin-RevId: 312830024 Change-Id: I76bc158d505792c31bf99fe040c7958418517173
diff --git a/quic/core/congestion_control/general_loss_algorithm.cc b/quic/core/congestion_control/general_loss_algorithm.cc index f00045e..eb0ed77 100644 --- a/quic/core/congestion_control/general_loss_algorithm.cc +++ b/quic/core/congestion_control/general_loss_algorithm.cc
@@ -56,7 +56,7 @@ QuicTime::Delta max_rtt = std::max(rtt_stats.previous_srtt(), rtt_stats.latest_rtt()); max_rtt = std::max(kAlarmGranularity, max_rtt); - QuicTime::Delta loss_delay = max_rtt + (max_rtt >> reordering_shift_); + const QuicTime::Delta loss_delay = max_rtt + (max_rtt >> reordering_shift_); QuicPacketNumber packet_number = unacked_packets.GetLeastUnacked(); auto it = unacked_packets.begin(); if (least_in_flight_.IsInitialized() && least_in_flight_ >= packet_number) { @@ -106,6 +106,10 @@ // Time threshold loss detection. QuicTime when_lost = it->sent_time + loss_delay; if (time < when_lost) { + if (time >= + it->sent_time + max_rtt + (max_rtt >> (reordering_shift_ + 1))) { + ++detection_stats.sent_packets_num_borderline_time_reorderings; + } loss_detection_timeout_ = when_lost; if (!least_in_flight_.IsInitialized()) { // At this point, packet_number is in flight and not detected as lost.
diff --git a/quic/core/congestion_control/general_loss_algorithm_test.cc b/quic/core/congestion_control/general_loss_algorithm_test.cc index 1a58ade..be33c25 100644 --- a/quic/core/congestion_control/general_loss_algorithm_test.cc +++ b/quic/core/congestion_control/general_loss_algorithm_test.cc
@@ -62,14 +62,17 @@ const AckedPacketVector& packets_acked, const std::vector<uint64_t>& losses_expected) { return VerifyLosses(largest_newly_acked, packets_acked, losses_expected, + quiche::QuicheOptional<QuicPacketCount>(), quiche::QuicheOptional<QuicPacketCount>()); } - void VerifyLosses(uint64_t largest_newly_acked, - const AckedPacketVector& packets_acked, - const std::vector<uint64_t>& losses_expected, - quiche::QuicheOptional<QuicPacketCount> - max_sequence_reordering_expected) { + void VerifyLosses( + uint64_t largest_newly_acked, + const AckedPacketVector& packets_acked, + const std::vector<uint64_t>& losses_expected, + quiche::QuicheOptional<QuicPacketCount> max_sequence_reordering_expected, + quiche::QuicheOptional<QuicPacketCount> + num_borderline_time_reorderings_expected) { unacked_packets_.MaybeUpdateLargestAckedOfPacketNumberSpace( APPLICATION_DATA, QuicPacketNumber(largest_newly_acked)); LostPacketVector lost_packets; @@ -80,6 +83,10 @@ EXPECT_EQ(stats.sent_packets_max_sequence_reordering, max_sequence_reordering_expected.value()); } + if (num_borderline_time_reorderings_expected.has_value()) { + EXPECT_EQ(stats.sent_packets_num_borderline_time_reorderings, + num_borderline_time_reorderings_expected.value()); + } ASSERT_EQ(losses_expected.size(), lost_packets.size()); for (size_t i = 0; i < losses_expected.size(); ++i) { EXPECT_EQ(lost_packets[i].packet_number, @@ -104,19 +111,19 @@ unacked_packets_.RemoveFromInFlight(QuicPacketNumber(2)); packets_acked.push_back(AckedPacket( QuicPacketNumber(2), kMaxOutgoingPacketSize, QuicTime::Zero())); - VerifyLosses(2, packets_acked, std::vector<uint64_t>{}, 1); + VerifyLosses(2, packets_acked, std::vector<uint64_t>{}, 1, 0); packets_acked.clear(); // No loss on two acks. unacked_packets_.RemoveFromInFlight(QuicPacketNumber(3)); packets_acked.push_back(AckedPacket( QuicPacketNumber(3), kMaxOutgoingPacketSize, QuicTime::Zero())); - VerifyLosses(3, packets_acked, std::vector<uint64_t>{}, 2); + VerifyLosses(3, packets_acked, std::vector<uint64_t>{}, 2, 0); packets_acked.clear(); // Loss on three acks. unacked_packets_.RemoveFromInFlight(QuicPacketNumber(4)); packets_acked.push_back(AckedPacket( QuicPacketNumber(4), kMaxOutgoingPacketSize, QuicTime::Zero())); - VerifyLosses(4, packets_acked, {1}, 3); + VerifyLosses(4, packets_acked, {1}, 3, 0); EXPECT_EQ(QuicTime::Zero(), loss_algorithm_.GetLossTimeout()); } @@ -176,7 +183,12 @@ EXPECT_EQ(clock_.Now() + 1.25 * rtt_stats_.smoothed_rtt(), loss_algorithm_.GetLossTimeout()); - clock_.AdvanceTime(1.25 * rtt_stats_.latest_rtt()); + clock_.AdvanceTime(1.13 * rtt_stats_.latest_rtt()); + // If reordering_shift increases by one we should have detected a loss. + VerifyLosses(2, packets_acked, {}, /*max_sequence_reordering_expected=*/1, + /*num_borderline_time_reorderings_expected=*/1); + + clock_.AdvanceTime(0.13 * rtt_stats_.latest_rtt()); VerifyLosses(2, packets_acked, {1}); EXPECT_EQ(QuicTime::Zero(), loss_algorithm_.GetLossTimeout()); }
diff --git a/quic/core/congestion_control/loss_detection_interface.h b/quic/core/congestion_control/loss_detection_interface.h index 8d91976..cd2bd72 100644 --- a/quic/core/congestion_control/loss_detection_interface.h +++ b/quic/core/congestion_control/loss_detection_interface.h
@@ -29,6 +29,7 @@ struct QUIC_NO_EXPORT DetectionStats { // Maximum sequence reordering observed in newly acked packets. QuicPacketCount sent_packets_max_sequence_reordering = 0; + QuicPacketCount sent_packets_num_borderline_time_reorderings = 0; }; // Called when a new ack arrives or the loss alarm fires.
diff --git a/quic/core/congestion_control/uber_loss_algorithm.cc b/quic/core/congestion_control/uber_loss_algorithm.cc index 0f294d6..6fd23d6 100644 --- a/quic/core/congestion_control/uber_loss_algorithm.cc +++ b/quic/core/congestion_control/uber_loss_algorithm.cc
@@ -54,6 +54,8 @@ overall_stats.sent_packets_max_sequence_reordering = std::max(overall_stats.sent_packets_max_sequence_reordering, stats.sent_packets_max_sequence_reordering); + overall_stats.sent_packets_num_borderline_time_reorderings += + stats.sent_packets_num_borderline_time_reorderings; } return overall_stats;
diff --git a/quic/core/quic_connection_stats.h b/quic/core/quic_connection_stats.h index 0aee970..397936b 100644 --- a/quic/core/quic_connection_stats.h +++ b/quic/core/quic_connection_stats.h
@@ -104,6 +104,9 @@ // Maximum sequence reordering observed from acked packets. QuicPacketCount sent_packets_max_sequence_reordering = 0; + // Number of times that a packet is not detected as lost per reordering_shift, + // but would have been if the reordering_shift increases by one. + QuicPacketCount sent_packets_num_borderline_time_reorderings = 0; // The following stats are used only in TcpCubicSender. // The number of loss events from TCP's perspective. Each loss event includes
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 7d32176..3941a3e 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -972,6 +972,9 @@ detection_stats.sent_packets_max_sequence_reordering; } + stats_->sent_packets_num_borderline_time_reorderings += + detection_stats.sent_packets_num_borderline_time_reorderings; + for (const LostPacket& packet : packets_lost_) { QuicTransmissionInfo* info = unacked_packets_.GetMutableTransmissionInfo(packet.packet_number);