gfe-relnote: Remove the dependency from QuicMemSliceSpan to QuicStreamSendBuffer and QuicMessageFrame. Refactor only, not protected. Currently, the (conceptually low level) QuicMemSliceSpan depends on (conceptually high level) QuicStreamSendBuffer and QuicMessageFrame classes, via QuicMemSliceSpan::SaveMemSlicesInSendBuffer and QuicMemSliceSpan::SaveMemSlicesAsMessageData. This change replace those functions by a new QuicMemSliceSpan::ConsumeAll method, which takes a callback instead of directly referring to QuicStreamSendBuffer and QuicMessageFrame. PiperOrigin-RevId: 240908280 Change-Id: I83c87037586252c1335f3f1e198e6b62915e5698
diff --git a/quic/core/frames/quic_message_frame.cc b/quic/core/frames/quic_message_frame.cc index c0de272..196f6e9 100644 --- a/quic/core/frames/quic_message_frame.cc +++ b/quic/core/frames/quic_message_frame.cc
@@ -6,6 +6,7 @@ #include "net/third_party/quiche/src/quic/core/quic_constants.h" #include "net/third_party/quiche/src/quic/platform/api/quic_logging.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_mem_slice.h" namespace quic { @@ -15,6 +16,15 @@ QuicMessageFrame::QuicMessageFrame(QuicMessageId message_id) : message_id(message_id), data(nullptr), message_length(0) {} +QuicMessageFrame::QuicMessageFrame(QuicMessageId message_id, + QuicMemSliceSpan span) + : message_id(message_id), data(nullptr), message_length(0) { + span.ConsumeAll([&](QuicMemSlice slice) { + message_length += slice.length(); + message_data.push_back(std::move(slice)); + }); +} + QuicMessageFrame::QuicMessageFrame(const char* data, QuicPacketLength length) : message_id(0), data(data), message_length(length) {}
diff --git a/quic/core/frames/quic_message_frame.h b/quic/core/frames/quic_message_frame.h index 4accff2..78dafe3 100644 --- a/quic/core/frames/quic_message_frame.h +++ b/quic/core/frames/quic_message_frame.h
@@ -9,6 +9,7 @@ #include "net/third_party/quiche/src/quic/platform/api/quic_containers.h" #include "net/third_party/quiche/src/quic/platform/api/quic_export.h" #include "net/third_party/quiche/src/quic/platform/api/quic_mem_slice.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_mem_slice_span.h" #include "net/third_party/quiche/src/quic/platform/api/quic_string_piece.h" namespace quic { @@ -18,6 +19,7 @@ struct QUIC_EXPORT_PRIVATE QuicMessageFrame { QuicMessageFrame(); explicit QuicMessageFrame(QuicMessageId message_id); + QuicMessageFrame(QuicMessageId message_id, QuicMemSliceSpan span); QuicMessageFrame(const char* data, QuicPacketLength length); QuicMessageFrame(const QuicMessageFrame& other) = delete;
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc index 2985059..9f0e700 100644 --- a/quic/core/quic_framer_test.cc +++ b/quic/core/quic_framer_test.cc
@@ -8432,11 +8432,8 @@ header.packet_number = kPacketNumber; QuicMemSliceStorage storage(nullptr, 0, nullptr, 0); - QuicMessageFrame frame(1); - MakeSpan(&allocator_, "message", &storage).SaveMemSlicesAsMessageData(&frame); - QuicMessageFrame frame2(2); - MakeSpan(&allocator_, "message2", &storage) - .SaveMemSlicesAsMessageData(&frame2); + QuicMessageFrame frame(1, MakeSpan(&allocator_, "message", &storage)); + QuicMessageFrame frame2(2, MakeSpan(&allocator_, "message2", &storage)); QuicFrames frames = {QuicFrame(&frame), QuicFrame(&frame2)}; // clang-format off
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc index 66b92b9..45f2651 100644 --- a/quic/core/quic_packet_creator_test.cc +++ b/quic/core/quic_packet_creator_test.cc
@@ -1616,23 +1616,21 @@ EXPECT_TRUE( creator_.HasRoomForMessageFrame(creator_.GetLargestMessagePayload())); std::string message(creator_.GetLargestMessagePayload(), 'a'); - QuicMessageFrame* message_frame = new QuicMessageFrame(1); - MakeSpan(&allocator_, message, &storage) - .SaveMemSlicesAsMessageData(message_frame); + QuicMessageFrame* message_frame = + new QuicMessageFrame(1, MakeSpan(&allocator_, message, &storage)); EXPECT_TRUE( creator_.AddSavedFrame(QuicFrame(message_frame), NOT_RETRANSMISSION)); EXPECT_TRUE(creator_.HasPendingFrames()); creator_.Flush(); - QuicMessageFrame* frame2 = new QuicMessageFrame(2); - MakeSpan(&allocator_, "message", &storage).SaveMemSlicesAsMessageData(frame2); + QuicMessageFrame* frame2 = + new QuicMessageFrame(2, MakeSpan(&allocator_, "message", &storage)); EXPECT_TRUE(creator_.AddSavedFrame(QuicFrame(frame2), NOT_RETRANSMISSION)); EXPECT_TRUE(creator_.HasPendingFrames()); // Verify if a new frame is added, 1 byte message length will be added. EXPECT_EQ(1u, creator_.ExpansionOnNewFrame()); - QuicMessageFrame* frame3 = new QuicMessageFrame(3); - MakeSpan(&allocator_, "message2", &storage) - .SaveMemSlicesAsMessageData(frame3); + QuicMessageFrame* frame3 = + new QuicMessageFrame(3, MakeSpan(&allocator_, "message2", &storage)); EXPECT_TRUE(creator_.AddSavedFrame(QuicFrame(frame3), NOT_RETRANSMISSION)); EXPECT_EQ(1u, creator_.ExpansionOnNewFrame()); creator_.Flush(); @@ -1643,16 +1641,15 @@ client_framer_.transport_version(), Perspective::IS_CLIENT); EXPECT_TRUE(creator_.ConsumeData(stream_id, &iov_, 1u, iov_.iov_len, 0u, 0u, false, false, NOT_RETRANSMISSION, &frame)); - QuicMessageFrame* frame4 = new QuicMessageFrame(4); - MakeSpan(&allocator_, "message", &storage).SaveMemSlicesAsMessageData(frame4); + QuicMessageFrame* frame4 = + new QuicMessageFrame(4, MakeSpan(&allocator_, "message", &storage)); EXPECT_TRUE(creator_.AddSavedFrame(QuicFrame(frame4), NOT_RETRANSMISSION)); EXPECT_TRUE(creator_.HasPendingFrames()); // Verify there is not enough room for largest payload. EXPECT_FALSE( creator_.HasRoomForMessageFrame(creator_.GetLargestMessagePayload())); // Add largest message will causes the flush of the stream frame. - QuicMessageFrame frame5(5); - MakeSpan(&allocator_, message, &storage).SaveMemSlicesAsMessageData(&frame5); + QuicMessageFrame frame5(5, MakeSpan(&allocator_, message, &storage)); EXPECT_FALSE(creator_.AddSavedFrame(QuicFrame(&frame5), NOT_RETRANSMISSION)); EXPECT_FALSE(creator_.HasPendingFrames()); } @@ -1667,10 +1664,10 @@ // Test all possible size of message frames. for (size_t message_size = 0; message_size <= creator_.GetLargestMessagePayload(); ++message_size) { - QuicMessageFrame* frame = new QuicMessageFrame(0); - MakeSpan(&allocator_, QuicStringPiece(message_buffer.data(), message_size), - &storage) - .SaveMemSlicesAsMessageData(frame); + QuicMessageFrame* frame = new QuicMessageFrame( + 0, MakeSpan(&allocator_, + QuicStringPiece(message_buffer.data(), message_size), + &storage)); EXPECT_TRUE(creator_.AddSavedFrame(QuicFrame(frame), NOT_RETRANSMISSION)); EXPECT_TRUE(creator_.HasPendingFrames());
diff --git a/quic/core/quic_packet_generator.cc b/quic/core/quic_packet_generator.cc index c2b3d48..257ca9b 100644 --- a/quic/core/quic_packet_generator.cc +++ b/quic/core/quic_packet_generator.cc
@@ -479,8 +479,7 @@ if (!packet_creator_.HasRoomForMessageFrame(message_length)) { packet_creator_.Flush(); } - QuicMessageFrame* frame = new QuicMessageFrame(message_id); - message.SaveMemSlicesAsMessageData(frame); + QuicMessageFrame* frame = new QuicMessageFrame(message_id, message); const bool success = packet_creator_.AddSavedFrame(QuicFrame(frame), next_transmission_type_); if (!success) {
diff --git a/quic/core/quic_stream.cc b/quic/core/quic_stream.cc index ef621fe..a3a63cd 100644 --- a/quic/core/quic_stream.cc +++ b/quic/core/quic_stream.cc
@@ -601,8 +601,7 @@ if (!span.empty()) { // Buffer all data if buffered data size is below limit. QuicStreamOffset offset = send_buffer_.stream_offset(); - consumed_data.bytes_consumed = - span.SaveMemSlicesInSendBuffer(&send_buffer_); + consumed_data.bytes_consumed = send_buffer_.SaveMemSliceSpan(span); if (offset > send_buffer_.stream_offset() || kMaxStreamLength < send_buffer_.stream_offset()) { QUIC_BUG << "Write too many data via stream " << id_;
diff --git a/quic/core/quic_stream_send_buffer.cc b/quic/core/quic_stream_send_buffer.cc index 5b66786..ead192d 100644 --- a/quic/core/quic_stream_send_buffer.cc +++ b/quic/core/quic_stream_send_buffer.cc
@@ -82,6 +82,11 @@ stream_offset_ += length; } +QuicByteCount QuicStreamSendBuffer::SaveMemSliceSpan(QuicMemSliceSpan span) { + return span.ConsumeAll( + [&](QuicMemSlice slice) { SaveMemSlice(std::move(slice)); }); +} + void QuicStreamSendBuffer::OnStreamDataConsumed(size_t bytes_consumed) { stream_bytes_written_ += bytes_consumed; stream_bytes_outstanding_ += bytes_consumed;
diff --git a/quic/core/quic_stream_send_buffer.h b/quic/core/quic_stream_send_buffer.h index 4bcac34..e34514b 100644 --- a/quic/core/quic_stream_send_buffer.h +++ b/quic/core/quic_stream_send_buffer.h
@@ -7,9 +7,11 @@ #include "net/third_party/quiche/src/quic/core/frames/quic_stream_frame.h" #include "net/third_party/quiche/src/quic/core/quic_interval_set.h" +#include "net/third_party/quiche/src/quic/core/quic_types.h" #include "net/third_party/quiche/src/quic/platform/api/quic_containers.h" #include "net/third_party/quiche/src/quic/platform/api/quic_iovec.h" #include "net/third_party/quiche/src/quic/platform/api/quic_mem_slice.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_mem_slice_span.h" namespace quic { @@ -72,6 +74,9 @@ // Save |slice| to send buffer. void SaveMemSlice(QuicMemSlice slice); + // Save all slices in |span| to send buffer. Return total bytes saved. + QuicByteCount SaveMemSliceSpan(QuicMemSliceSpan span); + // Called when |bytes_consumed| bytes has been consumed by the stream. void OnStreamDataConsumed(size_t bytes_consumed);
diff --git a/quic/core/quic_stream_send_buffer_test.cc b/quic/core/quic_stream_send_buffer_test.cc index 23a71fd..57e2bd1 100644 --- a/quic/core/quic_stream_send_buffer_test.cc +++ b/quic/core/quic_stream_send_buffer_test.cc
@@ -12,6 +12,7 @@ #include "net/third_party/quiche/src/quic/platform/api/quic_expect_bug.h" #include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_test_mem_slice_vector.h" #include "net/third_party/quiche/src/quic/test_tools/quic_stream_send_buffer_peer.h" #include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h" @@ -287,6 +288,38 @@ QuicStreamSendBufferPeer::CurrentWriteSlice(&send_buffer_)->offset); } +TEST_F(QuicStreamSendBufferTest, SaveMemSliceSpan) { + SimpleBufferAllocator allocator; + QuicStreamSendBuffer send_buffer(&allocator); + + char data[1024]; + std::vector<std::pair<char*, size_t>> buffers; + for (size_t i = 0; i < 10; ++i) { + buffers.push_back(std::make_pair(data, 1024)); + } + QuicTestMemSliceVector vector(buffers); + + EXPECT_EQ(10 * 1024u, send_buffer.SaveMemSliceSpan(vector.span())); + EXPECT_EQ(10u, send_buffer.size()); +} + +TEST_F(QuicStreamSendBufferTest, SaveEmptyMemSliceSpan) { + SimpleBufferAllocator allocator; + QuicStreamSendBuffer send_buffer(&allocator); + + char data[1024]; + std::vector<std::pair<char*, size_t>> buffers; + for (size_t i = 0; i < 10; ++i) { + buffers.push_back(std::make_pair(data, 1024)); + } + buffers.push_back(std::make_pair(nullptr, 0)); + QuicTestMemSliceVector vector(buffers); + + EXPECT_EQ(10 * 1024u, send_buffer.SaveMemSliceSpan(vector.span())); + // Verify the empty slice does not get saved. + EXPECT_EQ(10u, send_buffer.size()); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/platform/api/quic_mem_slice_span.h b/quic/platform/api/quic_mem_slice_span.h index 781382e..4fdf059 100644 --- a/quic/platform/api/quic_mem_slice_span.h +++ b/quic/platform/api/quic_mem_slice_span.h
@@ -29,16 +29,9 @@ ~QuicMemSliceSpan() = default; - // Save data buffers to |send_buffer| and returns the amount of saved data. - // |send_buffer| will hold a reference to all data buffer. - QuicByteCount SaveMemSlicesInSendBuffer(QuicStreamSendBuffer* send_buffer) { - return impl_.SaveMemSlicesInSendBuffer(send_buffer); - } - - // Save data buffers as message data in |message_frame|. |message_frame| will - // hold a reference to all data buffers. - void SaveMemSlicesAsMessageData(QuicMessageFrame* message_frame) { - impl_.SaveMemSlicesAsMessageData(message_frame); + template <typename ConsumeFunction> + QuicByteCount ConsumeAll(ConsumeFunction consume) { + return impl_.ConsumeAll(consume); } // Return data of the span at |index| by the form of a QuicStringPiece.
diff --git a/quic/platform/api/quic_mem_slice_span_test.cc b/quic/platform/api/quic_mem_slice_span_test.cc index b4c5e51..067a7a4 100644 --- a/quic/platform/api/quic_mem_slice_span_test.cc +++ b/quic/platform/api/quic_mem_slice_span_test.cc
@@ -5,7 +5,7 @@ #include "net/third_party/quiche/src/quic/platform/api/quic_mem_slice_span.h" #include "net/third_party/quiche/src/quic/core/quic_simple_buffer_allocator.h" -#include "net/third_party/quiche/src/quic/core/quic_stream_send_buffer.h" +#include "net/third_party/quiche/src/quic/core/quic_types.h" #include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test_mem_slice_vector.h" @@ -26,23 +26,20 @@ std::vector<std::pair<char*, size_t>> buffers_; }; -TEST_F(QuicMemSliceSpanImplTest, SaveDataInSendBuffer) { +TEST_F(QuicMemSliceSpanImplTest, ConsumeAll) { SimpleBufferAllocator allocator; - QuicStreamSendBuffer send_buffer(&allocator); QuicTestMemSliceVector vector(buffers_); - EXPECT_EQ(10 * 1024u, vector.span().SaveMemSlicesInSendBuffer(&send_buffer)); - EXPECT_EQ(10u, send_buffer.size()); -} + int num_slices = 0; + QuicByteCount bytes_consumed = + vector.span().ConsumeAll([&](QuicMemSlice slice) { + EXPECT_EQ(data_, slice.data()); + EXPECT_EQ(1024, slice.length()); + ++num_slices; + }); -TEST_F(QuicMemSliceSpanImplTest, SaveEmptyMemSliceInSendBuffer) { - SimpleBufferAllocator allocator; - QuicStreamSendBuffer send_buffer(&allocator); - buffers_.push_back(std::make_pair(nullptr, 0)); - QuicTestMemSliceVector vector(buffers_); - EXPECT_EQ(10 * 1024u, vector.span().SaveMemSlicesInSendBuffer(&send_buffer)); - // Verify the empty slice does not get saved. - EXPECT_EQ(10u, send_buffer.size()); + EXPECT_EQ(10 * 1024u, bytes_consumed); + EXPECT_EQ(10, num_slices); } } // namespace