Fix packet send time with udp gso packet writer, when the packet is bursted. protected by existing --gfe2_restart_flag_quic_support_release_time_for_gso. PiperOrigin-RevId: 319112843 Change-Id: I80f2cb98edb4f8d50f357992025691f8f2e26d9b
diff --git a/quic/core/batch_writer/quic_batch_writer_base.cc b/quic/core/batch_writer/quic_batch_writer_base.cc index 919f76b..46e230c 100644 --- a/quic/core/batch_writer/quic_batch_writer_base.cc +++ b/quic/core/batch_writer/quic_batch_writer_base.cc
@@ -8,6 +8,7 @@ #include "net/third_party/quiche/src/quic/platform/api/quic_export.h" #include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" #include "net/third_party/quiche/src/quic/platform/api/quic_ptr_util.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_server_stats.h" namespace quic { @@ -29,23 +30,50 @@ return result; } -uint64_t QuicBatchWriterBase::GetReleaseTime( +QuicBatchWriterBase::ReleaseTime QuicBatchWriterBase::GetReleaseTime( const PerPacketOptions* options) const { DCHECK(SupportsReleaseTime()); if (options == nullptr) { - return 0; + return {0, QuicTime::Delta::Zero()}; } + const uint64_t now = NowInNanosForReleaseTime(); + const uint64_t ideal_release_time = + now + options->release_time_delay.ToMicroseconds() * 1000; + if ((options->release_time_delay.IsZero() || options->allow_burst) && - !buffered_writes().empty()) { + !buffered_writes().empty() && + // If release time of buffered packets is in the past, flush buffered + // packets and buffer this packet at the ideal release time. + (buffered_writes().back().release_time >= now)) { // Send as soon as possible, but no sooner than the last buffered packet. - return buffered_writes().back().release_time; + const uint64_t actual_release_time = buffered_writes().back().release_time; + + const int64_t offset_ns = actual_release_time - ideal_release_time; + if (offset_ns >= 0) { + QUIC_SERVER_HISTOGRAM_TIMES("batch_writer_positive_release_time_offset", + offset_ns / 1000, 1, 100000, 50, + "Duration from ideal release time to actual " + "release time, in microseconds."); + } else { + QUIC_SERVER_HISTOGRAM_TIMES("batch_writer_negative_release_time_offset", + -offset_ns / 1000, 1, 100000, 50, + "Duration from actual release time to ideal " + "release time, in microseconds."); + } + + ReleaseTime result{actual_release_time, + QuicTime::Delta::FromMicroseconds(offset_ns / 1000)}; + + QUIC_DLOG(INFO) << "ideal_release_time:" << ideal_release_time + << ", actual_release_time:" << actual_release_time + << ", offset:" << result.release_time_offset; + return result; } // Send according to the release time delay. - return NowInNanosForReleaseTime() + - options->release_time_delay.ToMicroseconds() * 1000; + return {ideal_release_time, QuicTime::Delta::Zero()}; } WriteResult QuicBatchWriterBase::InternalWritePacket( @@ -58,10 +86,13 @@ return WriteResult(WRITE_STATUS_MSG_TOO_BIG, EMSGSIZE); } - uint64_t release_time = SupportsReleaseTime() ? GetReleaseTime(options) : 0; + ReleaseTime release_time = SupportsReleaseTime() + ? GetReleaseTime(options) + : ReleaseTime{0, QuicTime::Delta::Zero()}; - const CanBatchResult can_batch_result = CanBatch( - buffer, buf_len, self_address, peer_address, options, release_time); + const CanBatchResult can_batch_result = + CanBatch(buffer, buf_len, self_address, peer_address, options, + release_time.actual_release_time); bool buffered = false; bool flush = can_batch_result.must_flush; @@ -69,7 +100,8 @@ if (can_batch_result.can_batch) { QuicBatchWriterBuffer::PushResult push_result = batch_buffer_->PushBufferedWrite(buffer, buf_len, self_address, - peer_address, options, release_time); + peer_address, options, + release_time.actual_release_time); if (push_result.succeeded) { buffered = true; // If there's no space left after the packet is buffered, force a flush. @@ -81,12 +113,14 @@ } if (!flush) { - return WriteResult(WRITE_STATUS_OK, 0); + WriteResult result(WRITE_STATUS_OK, 0); + result.send_time_offset = release_time.release_time_offset; + return result; } size_t num_buffered_packets = buffered_writes().size(); const FlushImplResult flush_result = CheckedFlush(); - const WriteResult& result = flush_result.write_result; + WriteResult result = flush_result.write_result; QUIC_DVLOG(1) << "Internally flushed " << flush_result.num_packets_sent << " out of " << num_buffered_packets << " packets. WriteResult=" << result; @@ -103,18 +137,18 @@ buffered ? buffered_writes().size() : buffered_writes().size() + 1; batch_buffer().Clear(); - WriteResult result_with_dropped = result; - result_with_dropped.dropped_packets = + result.dropped_packets = dropped_packets > std::numeric_limits<uint16_t>::max() ? std::numeric_limits<uint16_t>::max() : static_cast<uint16_t>(dropped_packets); - return result_with_dropped; + return result; } if (!buffered) { QuicBatchWriterBuffer::PushResult push_result = batch_buffer_->PushBufferedWrite(buffer, buf_len, self_address, - peer_address, options, release_time); + peer_address, options, + release_time.actual_release_time); buffered = push_result.succeeded; // Since buffered_writes has been emptied, this write must have been @@ -125,6 +159,7 @@ << ", buf_len:" << buf_len; } + result.send_time_offset = release_time.release_time_offset; return result; }
diff --git a/quic/core/batch_writer/quic_batch_writer_base.h b/quic/core/batch_writer/quic_batch_writer_base.h index 0213894..0ba8113 100644 --- a/quic/core/batch_writer/quic_batch_writer_base.h +++ b/quic/core/batch_writer/quic_batch_writer_base.h
@@ -73,7 +73,14 @@ // Given the release delay in |options| and the state of |batch_buffer_|, get // the absolute release time. - uint64_t GetReleaseTime(const PerPacketOptions* options) const; + struct QUIC_NO_EXPORT ReleaseTime { + // The actual (absolute) release time. + uint64_t actual_release_time = 0; + // The difference between |actual_release_time| and ideal release time, + // which is (now + |options->release_time_delay|). + QuicTime::Delta release_time_offset = QuicTime::Delta::Zero(); + }; + ReleaseTime GetReleaseTime(const PerPacketOptions* options) const; struct QUIC_EXPORT_PRIVATE CanBatchResult { CanBatchResult(bool can_batch, bool must_flush)
diff --git a/quic/core/batch_writer/quic_gso_batch_writer.h b/quic/core/batch_writer/quic_gso_batch_writer.h index 64c7e02..d210cf7 100644 --- a/quic/core/batch_writer/quic_gso_batch_writer.h +++ b/quic/core/batch_writer/quic_gso_batch_writer.h
@@ -84,7 +84,8 @@ << ", peer_address: " << first.peer_address.ToString() << ", num_segments: " << buffered_writes().size() << ", total_bytes: " << total_bytes - << ", gso_size: " << gso_size; + << ", gso_size: " << gso_size + << ", release_time: " << first.release_time; // All segments in a GSO packet share the same fate - if the write failed, // none of them are sent, and it's not needed to call PopBufferedWrite().
diff --git a/quic/core/batch_writer/quic_gso_batch_writer_test.cc b/quic/core/batch_writer/quic_gso_batch_writer_test.cc index c7d695a..158783d 100644 --- a/quic/core/batch_writer/quic_gso_batch_writer_test.cc +++ b/quic/core/batch_writer/quic_gso_batch_writer_test.cc
@@ -42,6 +42,7 @@ using QuicGsoBatchWriter::GetReleaseTime; using QuicGsoBatchWriter::MaxSegments; using QuicGsoBatchWriter::QuicGsoBatchWriter; + using QuicGsoBatchWriter::ReleaseTime; static std::unique_ptr<TestQuicGsoBatchWriter> NewInstanceWithReleaseTimeSupport() { @@ -391,7 +392,7 @@ TEST_F(QuicGsoBatchWriterTest, ReleaseTimeNullOptions) { auto writer = TestQuicGsoBatchWriter::NewInstanceWithReleaseTimeSupport(); - EXPECT_EQ(0, writer->GetReleaseTime(nullptr)); + EXPECT_EQ(0, writer->GetReleaseTime(nullptr).actual_release_time); } TEST_F(QuicGsoBatchWriterTest, ReleaseTime) { @@ -402,17 +403,22 @@ TestPerPacketOptions options; EXPECT_TRUE(options.release_time_delay.IsZero()); EXPECT_FALSE(options.allow_burst); - EXPECT_EQ(MillisToNanos(1), writer->GetReleaseTime(&options)); + EXPECT_EQ(MillisToNanos(1), + writer->GetReleaseTime(&options).actual_release_time); // The 1st packet has no delay. - ASSERT_EQ(write_buffered, WritePacketWithOptions(writer.get(), &options)); + WriteResult result = WritePacketWithOptions(writer.get(), &options); + ASSERT_EQ(write_buffered, result); EXPECT_EQ(MillisToNanos(1), writer->buffered_writes().back().release_time); + EXPECT_EQ(result.send_time_offset, QuicTime::Delta::Zero()); // The 2nd packet has some delay, but allows burst. options.release_time_delay = QuicTime::Delta::FromMilliseconds(3); options.allow_burst = true; - ASSERT_EQ(write_buffered, WritePacketWithOptions(writer.get(), &options)); + result = WritePacketWithOptions(writer.get(), &options); + ASSERT_EQ(write_buffered, result); EXPECT_EQ(MillisToNanos(1), writer->buffered_writes().back().release_time); + EXPECT_EQ(result.send_time_offset, QuicTime::Delta::FromMilliseconds(-3)); // The 3rd packet has more delay and does not allow burst. // The first 2 packets are flushed due to different release time. @@ -424,14 +430,17 @@ })); options.release_time_delay = QuicTime::Delta::FromMilliseconds(5); options.allow_burst = false; - ASSERT_EQ(WriteResult(WRITE_STATUS_OK, 2700), - WritePacketWithOptions(writer.get(), &options)); + result = WritePacketWithOptions(writer.get(), &options); + ASSERT_EQ(WriteResult(WRITE_STATUS_OK, 2700), result); EXPECT_EQ(MillisToNanos(6), writer->buffered_writes().back().release_time); + EXPECT_EQ(result.send_time_offset, QuicTime::Delta::Zero()); // The 4th packet has same delay, but allows burst. options.allow_burst = true; - ASSERT_EQ(write_buffered, WritePacketWithOptions(writer.get(), &options)); + result = WritePacketWithOptions(writer.get(), &options); + ASSERT_EQ(write_buffered, result); EXPECT_EQ(MillisToNanos(6), writer->buffered_writes().back().release_time); + EXPECT_EQ(result.send_time_offset, QuicTime::Delta::Zero()); // The 5th packet has same delay, allows burst, but is shorter. // Packets 3,4 and 5 are flushed. @@ -442,7 +451,8 @@ return 0; })); options.allow_burst = true; - EXPECT_EQ(MillisToNanos(6), writer->GetReleaseTime(&options)); + EXPECT_EQ(MillisToNanos(6), + writer->GetReleaseTime(&options).actual_release_time); ASSERT_EQ(WriteResult(WRITE_STATUS_OK, 3000), writer->WritePacket(&packet_buffer_[0], 300, self_address_, peer_address_, &options)); @@ -452,8 +462,10 @@ // words, the release time should still be the same as packets 3-5. writer->ForceReleaseTimeMs(2); options.release_time_delay = QuicTime::Delta::FromMilliseconds(4); - ASSERT_EQ(write_buffered, WritePacketWithOptions(writer.get(), &options)); + result = WritePacketWithOptions(writer.get(), &options); + ASSERT_EQ(write_buffered, result); EXPECT_EQ(MillisToNanos(6), writer->buffered_writes().back().release_time); + EXPECT_EQ(result.send_time_offset, QuicTime::Delta::Zero()); } } // namespace
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 12a2e85..53dc7d9 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2728,6 +2728,14 @@ return false; } + if (result.status == WRITE_STATUS_OK) { + // packet_send_time is the ideal send time, if allow_burst is true, writer + // may have sent it earlier than that. + DCHECK((per_packet_options_ && per_packet_options_->allow_burst) || + result.send_time_offset.IsZero()); + packet_send_time = packet_send_time + result.send_time_offset; + } + if (debug_visitor_ != nullptr) { // Pass the write result to the visitor. debug_visitor_->OnPacketSent(*packet, packet->transmission_type,
diff --git a/quic/core/quic_types.h b/quic/core/quic_types.h index 0c736f9..b8e900d 100644 --- a/quic/core/quic_types.h +++ b/quic/core/quic_types.h
@@ -151,6 +151,11 @@ // Number of packets dropped as a result of this write. // Only used by batch writers. Otherwise always 0. uint16_t dropped_packets = 0; + // The delta between a packet's ideal and actual send time: + // actual_send_time = ideal_send_time + send_time_offset + // = (now + release_time_delay) + send_time_offset + // Only valid if |status| is WRITE_STATUS_OK. + QuicTime::Delta send_time_offset = QuicTime::Delta::Zero(); // TODO(wub): In some cases, WRITE_STATUS_ERROR may set an error_code and // WRITE_STATUS_BLOCKED_DATA_BUFFERED may set bytes_written. This may need // some cleaning up so that perhaps both values can be set and valid.