Move local_delayed_ack_time from QuicSentPacketManager to QuicReceivedPacketManager, set the delayed ack time to 1ms for Initial and Handshake packet number spaces when using v99, and use peer_delayed_ack_time instead of local_delayed_ack_time when with the MAD1 connection option. gfe-relnote: n/a (Refactor or v99 only) PiperOrigin-RevId: 261378188 Change-Id: I4bfed0bde6fa58506250b766294221f1aaf9fc29
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index df0d069..0b648e2 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1363,8 +1363,7 @@ uber_received_packet_manager_.MaybeUpdateAckTimeout( should_last_packet_instigate_acks_, last_decrypted_packet_level_, last_header_.packet_number, time_of_last_received_packet_, - clock_->ApproximateNow(), sent_packet_manager_.GetRttStats(), - sent_packet_manager_.local_max_ack_delay()); + clock_->ApproximateNow(), sent_packet_manager_.GetRttStats()); } else { QUIC_DLOG(INFO) << ENDPOINT << "Not updating ACK timeout for " << QuicUtils::EncryptionLevelToString(
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 45d31be..118e13b 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -683,6 +683,10 @@ // Returns the underlying sent packet manager. QuicSentPacketManager& sent_packet_manager() { return sent_packet_manager_; } + UberReceivedPacketManager& received_packet_manager() { + return uber_received_packet_manager_; + } + bool CanWrite(HasRetransmittableData retransmittable); // When the flusher is out of scope, only the outermost flusher will cause a
diff --git a/quic/core/quic_received_packet_manager.cc b/quic/core/quic_received_packet_manager.cc index 2994748..e21f4c4 100644 --- a/quic/core/quic_received_packet_manager.cc +++ b/quic/core/quic_received_packet_manager.cc
@@ -58,6 +58,8 @@ ack_decimation_delay_(kAckDecimationDelay), unlimited_ack_decimation_(false), fast_ack_after_quiescence_(false), + local_max_ack_delay_( + QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs)), ack_timeout_(QuicTime::Zero()), time_of_previous_received_packet_(QuicTime::Zero()), was_last_packet_missing_(false) { @@ -218,8 +220,7 @@ QuicPacketNumber last_received_packet_number, QuicTime time_of_last_received_packet, QuicTime now, - const RttStats* rtt_stats, - QuicTime::Delta local_max_ack_delay) { + const RttStats* rtt_stats) { if (!ack_frame_updated_) { // ACK frame has not been updated, nothing to do. return; @@ -251,7 +252,7 @@ // Wait for the minimum of the ack decimation delay or the delayed ack time // before sending an ack. QuicTime::Delta ack_delay = std::min( - local_max_ack_delay, rtt_stats->min_rtt() * ack_decimation_delay_); + local_max_ack_delay_, rtt_stats->min_rtt() * ack_decimation_delay_); if (fast_ack_after_quiescence_ && now - time_of_previous_received_packet_ > rtt_stats->SmoothedOrInitialRtt()) { // Ack the first packet out of queiscence faster, because QUIC does @@ -273,7 +274,7 @@ // or TLP packets, which we'd like to acknowledge quickly. MaybeUpdateAckTimeoutTo(now + QuicTime::Delta::FromMilliseconds(1)); } else { - MaybeUpdateAckTimeoutTo(now + local_max_ack_delay); + MaybeUpdateAckTimeoutTo(now + local_max_ack_delay_); } }
diff --git a/quic/core/quic_received_packet_manager.h b/quic/core/quic_received_packet_manager.h index aaae7f1..293b03c 100644 --- a/quic/core/quic_received_packet_manager.h +++ b/quic/core/quic_received_packet_manager.h
@@ -64,8 +64,7 @@ QuicPacketNumber last_received_packet_number, QuicTime time_of_last_received_packet, QuicTime now, - const RttStats* rtt_stats, - QuicTime::Delta local_max_ack_delay); + const RttStats* rtt_stats); // Resets ACK related states, called after an ACK is successfully sent. void ResetAckStates(); @@ -119,6 +118,11 @@ ack_frequency_before_ack_decimation_ = new_value; } + QuicTime::Delta local_max_ack_delay() const { return local_max_ack_delay_; } + void set_local_max_ack_delay(QuicTime::Delta local_max_ack_delay) { + local_max_ack_delay_ = local_max_ack_delay; + } + QuicTime ack_timeout() const { return ack_timeout_; } private: @@ -172,6 +176,9 @@ // was received. bool fast_ack_after_quiescence_; + // The local node's maximum ack delay time. This is the maximum amount of + // time to wait before sending an acknowledgement. + QuicTime::Delta local_max_ack_delay_; // Time that an ACK needs to be sent. 0 means no ACK is pending. Used when // decide_when_to_send_acks_ is true. QuicTime ack_timeout_;
diff --git a/quic/core/quic_received_packet_manager_test.cc b/quic/core/quic_received_packet_manager_test.cc index 86ac933..db5e260 100644 --- a/quic/core/quic_received_packet_manager_test.cc +++ b/quic/core/quic_received_packet_manager_test.cc
@@ -90,7 +90,7 @@ received_manager_.MaybeUpdateAckTimeout( should_last_packet_instigate_acks, QuicPacketNumber(last_received_packet_number), clock_.ApproximateNow(), - clock_.ApproximateNow(), &rtt_stats_, kDelayedAckTime); + clock_.ApproximateNow(), &rtt_stats_); } void CheckAckTimeout(QuicTime time) {
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 7427b73..fb0e8de 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -111,8 +111,6 @@ ietf_style_2x_tlp_(false), largest_mtu_acked_(0), handshake_confirmed_(false), - local_max_ack_delay_( - QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs)), peer_max_ack_delay_( QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs)), rtt_updated_(false),
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index b08704d..010181c 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -355,17 +355,11 @@ return pending_timer_transmission_count_; } - QuicTime::Delta local_max_ack_delay() const { return local_max_ack_delay_; } - - void set_local_max_ack_delay(QuicTime::Delta local_max_ack_delay) { - // The delayed ack time should never be more than one half the min RTO time. - DCHECK_LE(local_max_ack_delay, (min_rto_timeout_ * 0.5)); - local_max_ack_delay_ = local_max_ack_delay; - } - QuicTime::Delta peer_max_ack_delay() const { return peer_max_ack_delay_; } void set_peer_max_ack_delay(QuicTime::Delta peer_max_ack_delay) { + // The delayed ack time should never be more than one half the min RTO time. + DCHECK_LE(peer_max_ack_delay, (min_rto_timeout_ * 0.5)); peer_max_ack_delay_ = peer_max_ack_delay; } @@ -615,10 +609,6 @@ QuicPacketNumber largest_packets_peer_knows_is_acked_[NUM_PACKET_NUMBER_SPACES]; - // The local node's maximum ack delay time. This is the maximum amount of - // time to wait before sending an acknowledgement. - QuicTime::Delta local_max_ack_delay_; - // The maximum ACK delay time that the peer uses. Initialized to be the // same as local_max_ack_delay_, may be changed via transport parameter // negotiation.
diff --git a/quic/core/uber_received_packet_manager.cc b/quic/core/uber_received_packet_manager.cc index 6df7490..78cf359 100644 --- a/quic/core/uber_received_packet_manager.cc +++ b/quic/core/uber_received_packet_manager.cc
@@ -4,6 +4,7 @@ #include "net/third_party/quiche/src/quic/core/uber_received_packet_manager.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_bug_tracker.h" @@ -77,19 +78,18 @@ QuicPacketNumber last_received_packet_number, QuicTime time_of_last_received_packet, QuicTime now, - const RttStats* rtt_stats, - QuicTime::Delta local_max_ack_delay) { + const RttStats* rtt_stats) { if (!supports_multiple_packet_number_spaces_) { received_packet_managers_[0].MaybeUpdateAckTimeout( should_last_packet_instigate_acks, last_received_packet_number, - time_of_last_received_packet, now, rtt_stats, local_max_ack_delay); + time_of_last_received_packet, now, rtt_stats); return; } received_packet_managers_[QuicUtils::GetPacketNumberSpace( decrypted_packet_level)] - .MaybeUpdateAckTimeout( - should_last_packet_instigate_acks, last_received_packet_number, - time_of_last_received_packet, now, rtt_stats, local_max_ack_delay); + .MaybeUpdateAckTimeout(should_last_packet_instigate_acks, + last_received_packet_number, + time_of_last_received_packet, now, rtt_stats); } void UberReceivedPacketManager::ResetAckStates( @@ -112,6 +112,12 @@ "packet has been received."; return; } + // In IETF QUIC, the peer is expected to acknowledge packets in Initial and + // Handshake packets with minimal delay. + received_packet_managers_[INITIAL_DATA].set_local_max_ack_delay( + QuicTime::Delta::FromMilliseconds(1)); + received_packet_managers_[HANDSHAKE_DATA].set_local_max_ack_delay( + QuicTime::Delta::FromMilliseconds(1)); supports_multiple_packet_number_spaces_ = true; } @@ -207,6 +213,23 @@ } } +QuicTime::Delta UberReceivedPacketManager::max_ack_delay() { + if (!supports_multiple_packet_number_spaces_) { + return received_packet_managers_[0].local_max_ack_delay(); + } + return received_packet_managers_[APPLICATION_DATA].local_max_ack_delay(); +} + +void UberReceivedPacketManager::set_max_ack_delay( + QuicTime::Delta max_ack_delay) { + if (!supports_multiple_packet_number_spaces_) { + received_packet_managers_[0].set_local_max_ack_delay(max_ack_delay); + return; + } + received_packet_managers_[APPLICATION_DATA].set_local_max_ack_delay( + max_ack_delay); +} + void UberReceivedPacketManager::set_save_timestamps(bool save_timestamps) { for (auto& received_packet_manager : received_packet_managers_) { received_packet_manager.set_save_timestamps(save_timestamps);
diff --git a/quic/core/uber_received_packet_manager.h b/quic/core/uber_received_packet_manager.h index c442804..14ba868 100644 --- a/quic/core/uber_received_packet_manager.h +++ b/quic/core/uber_received_packet_manager.h
@@ -48,8 +48,7 @@ QuicPacketNumber last_received_packet_number, QuicTime time_of_last_received_packet, QuicTime now, - const RttStats* rtt_stats, - QuicTime::Delta local_max_ack_delay); + const RttStats* rtt_stats); // Resets ACK related states, called after an ACK is successfully sent. void ResetAckStates(EncryptionLevel encryption_level); @@ -89,6 +88,10 @@ void set_max_ack_ranges(size_t max_ack_ranges); + // Get and set the max ack delay to use for application data. + QuicTime::Delta max_ack_delay(); + void set_max_ack_delay(QuicTime::Delta max_ack_delay); + void set_save_timestamps(bool save_timestamps); private:
diff --git a/quic/core/uber_received_packet_manager_test.cc b/quic/core/uber_received_packet_manager_test.cc index 64dbb17..9372da8 100644 --- a/quic/core/uber_received_packet_manager_test.cc +++ b/quic/core/uber_received_packet_manager_test.cc
@@ -98,7 +98,7 @@ manager_->MaybeUpdateAckTimeout( should_last_packet_instigate_acks, decrypted_packet_level, QuicPacketNumber(last_received_packet_number), clock_.ApproximateNow(), - clock_.ApproximateNow(), &rtt_stats_, kDelayedAckTime); + clock_.ApproximateNow(), &rtt_stats_); } void CheckAckTimeout(QuicTime time) { @@ -378,6 +378,20 @@ CheckAckTimeout(ack_time); } +TEST_F(UberReceivedPacketManagerTest, SendDelayedMaxAckDelay) { + EXPECT_FALSE(HasPendingAck()); + QuicTime::Delta max_ack_delay = QuicTime::Delta::FromMilliseconds(100); + manager_->set_max_ack_delay(max_ack_delay); + QuicTime ack_time = clock_.ApproximateNow() + max_ack_delay; + + RecordPacketReceipt(1, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 1); + CheckAckTimeout(ack_time); + // Simulate delayed ack alarm firing. + clock_.AdvanceTime(max_ack_delay); + CheckAckTimeout(clock_.ApproximateNow()); +} + TEST_F(UberReceivedPacketManagerTest, SendDelayedAckDecimation) { EXPECT_FALSE(HasPendingAck()); UberReceivedPacketManagerPeer::SetAckMode(manager_.get(), ACK_DECIMATION); @@ -765,7 +779,12 @@ MaybeUpdateAckTimeout(kInstigateAck, ENCRYPTION_HANDSHAKE, 3); EXPECT_TRUE(HasPendingAck()); // Delayed ack is scheduled. - CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); + CheckAckTimeout(clock_.ApproximateNow() + + QuicTime::Delta::FromMilliseconds(1)); + // Send delayed handshake data ACK. + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(1)); + CheckAckTimeout(clock_.ApproximateNow()); + EXPECT_FALSE(HasPendingAck()); RecordPacketReceipt(ENCRYPTION_FORWARD_SECURE, 3); MaybeUpdateAckTimeout(kInstigateAck, ENCRYPTION_FORWARD_SECURE, 3); @@ -777,12 +796,6 @@ MaybeUpdateAckTimeout(kInstigateAck, ENCRYPTION_FORWARD_SECURE, 2); // Application data ACK should be sent immediately. CheckAckTimeout(clock_.ApproximateNow()); - // Delayed ACK of handshake data is pending. - CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); - - // Send delayed handshake data ACK. - clock_.AdvanceTime(kDelayedAckTime); - CheckAckTimeout(clock_.ApproximateNow()); EXPECT_FALSE(HasPendingAck()); }
diff --git a/quic/quartc/quartc_factory.cc b/quic/quartc/quartc_factory.cc index 8a69ab5..6742b89 100644 --- a/quic/quartc/quartc_factory.cc +++ b/quic/quartc/quartc_factory.cc
@@ -8,6 +8,7 @@ #include "net/third_party/quiche/src/quic/core/quic_utils.h" #include "net/third_party/quiche/src/quic/core/tls_client_handshaker.h" #include "net/third_party/quiche/src/quic/core/tls_server_handshaker.h" +#include "net/third_party/quiche/src/quic/core/uber_received_packet_manager.h" #include "net/third_party/quiche/src/quic/platform/api/quic_ptr_util.h" #include "net/third_party/quiche/src/quic/platform/api/quic_socket_address.h" #include "net/third_party/quiche/src/quic/quartc/quartc_connection_helper.h" @@ -46,8 +47,8 @@ // Quartc sets its own ack delay; get that ack delay and copy it over // to the QuicConfig so that it can be properly advertised to the peer // via transport parameter negotiation. - quic_config.SetMaxAckDelayToSendMs(quic_connection->sent_packet_manager() - .local_max_ack_delay() + quic_config.SetMaxAckDelayToSendMs(quic_connection->received_packet_manager() + .max_ack_delay() .ToMilliseconds()); return QuicMakeUnique<QuartcClientSession>( @@ -202,6 +203,8 @@ QuicSentPacketManager& sent_packet_manager = quic_connection->sent_packet_manager(); + UberReceivedPacketManager& received_packet_manager = + quic_connection->received_packet_manager(); // Default delayed ack time is 25ms. // If data packets are sent less often (e.g. because p-time was modified), @@ -213,7 +216,7 @@ // The p-time can go up to as high as 120ms, and when it does, it's // when the low overhead is the most important thing. Ideally it should be // above 120ms, but it cannot be higher than 0.5*RTO, which equals to 100ms. - sent_packet_manager.set_local_max_ack_delay( + received_packet_manager.set_max_ack_delay( QuicTime::Delta::FromMilliseconds(100)); sent_packet_manager.set_peer_max_ack_delay( QuicTime::Delta::FromMilliseconds(100));