Limit the amount of buffered QUIC receive timestamps Currently, this limit is only enforced in the framer. PiperOrigin-RevId: 1015602175
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index 9ec9e43..d32e051 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -6499,7 +6499,7 @@ } max_timestamps = std::min(max_timestamps, kMaxReceiveTimestampsToSend); - uber_received_packet_manager_.set_save_timestamps(true); + uber_received_packet_manager_.SaveReceiveTimestamps(max_timestamps); framer_.set_peer_max_receive_timestamps_per_ack(max_timestamps); framer_.set_peer_receive_timestamps_exponent(exponent); }
diff --git a/quiche/quic/core/quic_received_packet_manager.cc b/quiche/quic/core/quic_received_packet_manager.cc index 2878af8..9dfcc2c 100644 --- a/quiche/quic/core/quic_received_packet_manager.cc +++ b/quiche/quic/core/quic_received_packet_manager.cc
@@ -45,7 +45,7 @@ : ack_frame_updated_(false), max_ack_ranges_(0), time_largest_observed_(QuicTime::Zero()), - save_timestamps_(false), + max_timestamps_(0), stats_(stats), num_retransmittable_packets_received_since_last_ack_sent_(0), min_received_before_ack_decimation_(kMinReceivedBeforeAckDecimation), @@ -111,10 +111,10 @@ ack_frame_.packets.Add(packet_number); MaybeTrimAckRanges(); - if (save_timestamps_) { + if (save_timestamps()) { if (!receive_timestamp_basis_.IsInitialized()) { QUIC_BUG(missing_receive_timestamp_basis) - << "`save_timestamps_` is set to true, but " + << "`SaveReceiveTimestamps` was called, but " "`receive_timestamp_basis_` is not set"; } @@ -133,6 +133,14 @@ << ack_frame_.received_packet_times.back().second.ToDebuggingValue() << " to " << receive_time->ToDebuggingValue(); } else { + if (ack_frame_.received_packet_times.size() >= max_timestamps_) { + // Drop the oldest timestamp to keep at most `max_timestamps_` most + // recent entries, as recommended in the draft. + // Because the limit for `max_timestamps_` is fairly small, erasing from + // the front is fast despite `received_packet_times` being a vector. + ack_frame_.received_packet_times.erase( + ack_frame_.received_packet_times.begin()); + } ack_frame_.received_packet_times.push_back( std::make_pair(packet_number, *receive_time)); } @@ -483,4 +491,14 @@ reordering_threshold_ = frame.reordering_threshold; } +void QuicReceivedPacketManager::SaveReceiveTimestamps(size_t max_timestamps) { + max_timestamps_ = max_timestamps; + if (ack_frame_.received_packet_times.size() > max_timestamps_) { + ack_frame_.received_packet_times.erase( + ack_frame_.received_packet_times.begin(), + ack_frame_.received_packet_times.begin() + + (ack_frame_.received_packet_times.size() - max_timestamps_)); + } +} + } // namespace quic
diff --git a/quiche/quic/core/quic_received_packet_manager.h b/quiche/quic/core/quic_received_packet_manager.h index 75615db..459ba08 100644 --- a/quiche/quic/core/quic_received_packet_manager.h +++ b/quiche/quic/core/quic_received_packet_manager.h
@@ -113,9 +113,8 @@ max_ack_ranges_ = max_ack_ranges; } - void set_save_timestamps(bool save_timestamps) { - save_timestamps_ = save_timestamps; - } + void SaveReceiveTimestamps(size_t max_timestamps); + bool save_timestamps() const { return max_timestamps_ > 0; } void set_receive_timestamp_basis(QuicTime receive_timestamp_basis) { receive_timestamp_basis_ = receive_timestamp_basis; @@ -180,8 +179,9 @@ // Needed for calculating ack_delay_time. QuicTime time_largest_observed_; - // If true, save timestamps in the ack_frame_. - bool save_timestamps_; + // Maximum number of receive timestamps to save in ack_frame_. 0 means + // timestamps are not saved. + size_t max_timestamps_; // Least packet number received from peer. QuicPacketNumber least_received_packet_number_;
diff --git a/quiche/quic/core/quic_received_packet_manager_test.cc b/quiche/quic/core/quic_received_packet_manager_test.cc index b45a0a8..a4f775e 100644 --- a/quiche/quic/core/quic_received_packet_manager_test.cc +++ b/quiche/quic/core/quic_received_packet_manager_test.cc
@@ -48,7 +48,7 @@ clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1)); rtt_stats_.UpdateRtt(kMinRttMs, QuicTime::Delta::Zero(), QuicTime::Zero()); received_manager_.set_receive_timestamp_basis(clock_.ApproximateNow()); - received_manager_.set_save_timestamps(true); + received_manager_.SaveReceiveTimestamps(kMaxReceiveTimestampsToSend); } void RecordPacketReceipt(uint64_t packet_number) { @@ -202,7 +202,7 @@ } TEST_F(QuicReceivedPacketManagerTest, TrimmedAckRangesRemovedFromTimestamps) { - received_manager_.set_save_timestamps(true); + received_manager_.SaveReceiveTimestamps(kMaxReceiveTimestampsToSend); received_manager_.set_max_ack_ranges(2); RecordPacketReceipt( @@ -269,7 +269,7 @@ } TEST_F(QuicReceivedPacketManagerTest, SaveOutOfOrderPackets) { - received_manager_.set_save_timestamps(true); + received_manager_.SaveReceiveTimestamps(kMaxReceiveTimestampsToSend); EXPECT_FALSE(received_manager_.ack_frame_updated()); RecordPacketReceipt(1, clock_.ApproximateNow()); EXPECT_TRUE(received_manager_.ack_frame_updated()); @@ -283,6 +283,46 @@ EXPECT_EQ(3u, received_manager_.ack_frame().received_packet_times.size()); } +TEST_F(QuicReceivedPacketManagerTest, EvictOldestTimestampsWhenLimitExceeded) { + constexpr size_t kMaxTimestamps = 3; + received_manager_.SaveReceiveTimestamps(kMaxTimestamps); + + for (uint64_t i = 1; i <= 5; ++i) { + RecordPacketReceipt( + i, clock_.ApproximateNow() + QuicTime::Delta::FromMilliseconds(i)); + } + + // Only the 3 most recent timestamps (packets 3, 4, 5) should be retained. + const auto& timestamps = received_manager_.ack_frame().received_packet_times; + ASSERT_EQ(kMaxTimestamps, timestamps.size()); + EXPECT_EQ(QuicPacketNumber(3), timestamps[0].first); + EXPECT_EQ(QuicPacketNumber(4), timestamps[1].first); + EXPECT_EQ(QuicPacketNumber(5), timestamps[2].first); +} + +TEST_F(QuicReceivedPacketManagerTest, TruncateExistingTimestampsOnLowerLimit) { + received_manager_.SaveReceiveTimestamps(5); + for (uint64_t i = 1; i <= 5; ++i) { + RecordPacketReceipt( + i, clock_.ApproximateNow() + QuicTime::Delta::FromMilliseconds(i)); + } + EXPECT_EQ(5u, received_manager_.ack_frame().received_packet_times.size()); + + received_manager_.SaveReceiveTimestamps(2); + const auto& timestamps = received_manager_.ack_frame().received_packet_times; + ASSERT_EQ(2u, timestamps.size()); + EXPECT_EQ(QuicPacketNumber(4), timestamps[0].first); + EXPECT_EQ(QuicPacketNumber(5), timestamps[1].first); + + received_manager_.SaveReceiveTimestamps(0); + EXPECT_FALSE(received_manager_.save_timestamps()); + EXPECT_TRUE(received_manager_.ack_frame().received_packet_times.empty()); + + RecordPacketReceipt( + 6, clock_.ApproximateNow() + QuicTime::Delta::FromMilliseconds(6)); + EXPECT_TRUE(received_manager_.ack_frame().received_packet_times.empty()); +} + TEST_F(QuicReceivedPacketManagerTest, HasMissingPackets) { EXPECT_QUIC_BUG(received_manager_.PeerFirstSendingPacketNumber(), "No packets have been received yet");
diff --git a/quiche/quic/core/uber_received_packet_manager.cc b/quiche/quic/core/uber_received_packet_manager.cc index 6a6432a..075ca54 100644 --- a/quiche/quic/core/uber_received_packet_manager.cc +++ b/quiche/quic/core/uber_received_packet_manager.cc
@@ -233,14 +233,14 @@ } } -void UberReceivedPacketManager::set_save_timestamps(bool save_timestamps) { - if (!supports_multiple_packet_number_spaces_ && save_timestamps) { +void UberReceivedPacketManager::SaveReceiveTimestamps(size_t max_timestamps) { + if (!supports_multiple_packet_number_spaces_ && max_timestamps > 0) { QUIC_BUG(recv_timestamps_with_gquic) << "Trying to enable QUIC receive timestamps for non-IETF QUIC"; return; } - received_packet_managers_[APPLICATION_DATA].set_save_timestamps( - save_timestamps); + received_packet_managers_[APPLICATION_DATA].SaveReceiveTimestamps( + max_timestamps); } void UberReceivedPacketManager::set_receive_timestamp_basis(
diff --git a/quiche/quic/core/uber_received_packet_manager.h b/quiche/quic/core/uber_received_packet_manager.h index 21a769b..5606681 100644 --- a/quiche/quic/core/uber_received_packet_manager.h +++ b/quiche/quic/core/uber_received_packet_manager.h
@@ -95,7 +95,7 @@ void OnAckFrequencyFrame(const QuicAckFrequencyFrame& frame); - void set_save_timestamps(bool save_timestamps); + void SaveReceiveTimestamps(size_t max_timestamps); void set_receive_timestamp_basis(QuicTime receive_timestamp_basis); private:
diff --git a/quiche/quic/core/uber_received_packet_manager_test.cc b/quiche/quic/core/uber_received_packet_manager_test.cc index 545d6d8..4b65a71 100644 --- a/quiche/quic/core/uber_received_packet_manager_test.cc +++ b/quiche/quic/core/uber_received_packet_manager_test.cc
@@ -159,7 +159,7 @@ TEST_F(UberReceivedPacketManagerTest, GetUpdatedAckFrame) { manager_->EnableMultiplePacketNumberSpacesSupport(Perspective::IS_CLIENT); - manager_->set_save_timestamps(true); + manager_->SaveReceiveTimestamps(1000); QuicTime two_ms = clock_.ApproximateNow() + QuicTime::Delta::FromMilliseconds(2); @@ -237,7 +237,7 @@ TEST_F(UberReceivedPacketManagerTest, IgnoreOutOfOrderTimestamps) { manager_->EnableMultiplePacketNumberSpacesSupport(Perspective::IS_CLIENT); - manager_->set_save_timestamps(true); + manager_->SaveReceiveTimestamps(1000); EXPECT_FALSE(manager_->IsAckFrameUpdated()); RecordPacketReceipt(1, clock_.ApproximateNow()); @@ -254,7 +254,7 @@ } TEST_F(UberReceivedPacketManagerTest, CannotEnableTimestampsForNonIetfQuic) { - EXPECT_QUIC_BUG(manager_->set_save_timestamps(true), + EXPECT_QUIC_BUG(manager_->SaveReceiveTimestamps(1000), "Trying to enable QUIC receive timestamps for non-IETF QUIC"); } @@ -606,7 +606,7 @@ TEST_F(UberReceivedPacketManagerTest, SaveTimestampsOnlyForApplicationDataPacketNumberSpace) { manager_->EnableMultiplePacketNumberSpacesSupport(Perspective::IS_SERVER); - manager_->set_save_timestamps(true); + manager_->SaveReceiveTimestamps(1000); const QuicTime basis = clock_.ApproximateNow(); const QuicTime initial_time = basis + QuicTime::Delta::FromMilliseconds(1);