Coalesce adjacent stream frames. gfe-relnote: protected by gfe2_reloadable_flag_quic_coalesce_stream_frames. PiperOrigin-RevId: 275506579 Change-Id: I94697cc2ceaffc05d03511a342fcf8743780dd77
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index f90f3c7..c684916 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -310,7 +310,7 @@ // Write body. QUIC_DLOG(INFO) << ENDPOINT << "Stream " << id() << " is writing DATA frame payload of length " - << data.length(); + << data.length() << " with fin " << fin; WriteOrBufferData(data, fin, nullptr); } @@ -1056,7 +1056,7 @@ QUIC_DLOG(INFO) << ENDPOINT << "Stream " << id() << " is writing HEADERS frame payload of length " - << encoded_headers.length(); + << encoded_headers.length() << " with fin " << fin; WriteOrBufferData(encoded_headers, fin, nullptr); QuicSpdySession::LogHeaderCompressionRatioHistogram(
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index 07fa4b1..ee2dc02 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -11,7 +11,9 @@ #include <utility> #include "net/third_party/quiche/src/quic/core/crypto/crypto_protocol.h" +#include "net/third_party/quiche/src/quic/core/frames/quic_frame.h" #include "net/third_party/quiche/src/quic/core/frames/quic_path_challenge_frame.h" +#include "net/third_party/quiche/src/quic/core/frames/quic_stream_frame.h" #include "net/third_party/quiche/src/quic/core/quic_connection_id.h" #include "net/third_party/quiche/src/quic/core/quic_constants.h" #include "net/third_party/quiche/src/quic/core/quic_data_writer.h" @@ -1369,6 +1371,14 @@ QUIC_ATTEMPT_TO_SEND_UNENCRYPTED_STREAM_DATA, error_details); return false; } + + if (GetQuicReloadableFlag(quic_coalesce_stream_frames) && + frame.type == STREAM_FRAME && + MaybeCoalesceStreamFrame(frame.stream_frame)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_coalesce_stream_frames); + return true; + } + size_t frame_len = framer_->GetSerializedFrameLength( frame, BytesFree(), queued_frames_.empty(), /* last_frame_in_packet= */ true, GetPacketNumberLength()); @@ -1412,6 +1422,36 @@ return true; } +bool QuicPacketCreator::MaybeCoalesceStreamFrame(const QuicStreamFrame& frame) { + if (queued_frames_.empty() || queued_frames_.back().type != STREAM_FRAME) { + return false; + } + QuicStreamFrame* candidate = &queued_frames_.back().stream_frame; + if (candidate->stream_id != frame.stream_id || + candidate->offset + candidate->data_length != frame.offset || + frame.data_length > BytesFree()) { + return false; + } + candidate->data_length += frame.data_length; + candidate->fin = frame.fin; + + // The back of retransmittable frames must be the same as the original + // queued frames' back. + DCHECK_EQ(packet_.retransmittable_frames.back().type, STREAM_FRAME); + QuicStreamFrame* retransmittable = + &packet_.retransmittable_frames.back().stream_frame; + DCHECK_EQ(retransmittable->stream_id, frame.stream_id); + DCHECK_EQ(retransmittable->offset + retransmittable->data_length, + frame.offset); + retransmittable->data_length = candidate->data_length; + retransmittable->fin = candidate->fin; + packet_size_ += frame.data_length; + if (debug_delegate_ != nullptr) { + debug_delegate_->OnStreamFrameCoalesced(*candidate); + } + return true; +} + void QuicPacketCreator::MaybeAddPadding() { // The current packet should have no padding bytes because padding is only // added when this method is called just before the packet is serialized.
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h index f202af0..9b4b665 100644 --- a/quic/core/quic_packet_creator.h +++ b/quic/core/quic_packet_creator.h
@@ -13,6 +13,7 @@ #include <utility> #include <vector> +#include "net/third_party/quiche/src/quic/core/frames/quic_stream_frame.h" #include "net/third_party/quiche/src/quic/core/quic_framer.h" #include "net/third_party/quiche/src/quic/core/quic_packets.h" #include "net/third_party/quiche/src/quic/core/quic_pending_retransmission.h" @@ -60,6 +61,10 @@ // Called when a frame has been added to the current packet. virtual void OnFrameAddedToPacket(const QuicFrame& /*frame*/) {} + + // Called when a stream frame is coalesced with an existing stream frame. + // |frame| is the new stream frame. + virtual void OnStreamFrameCoalesced(const QuicStreamFrame& /*frame*/) {} }; QuicPacketCreator(QuicConnectionId server_connection_id, @@ -468,6 +473,10 @@ // Clears all fields of packet_ that should be cleared between serializations. void ClearPacket(); + // Tries to coalesce |frame| with the back of |queued_frames_|. + // Returns true on success. + bool MaybeCoalesceStreamFrame(const QuicStreamFrame& frame); + // Returns true if a diversification nonce should be included in the current // packet's header. bool IncludeNonceInPublicHeader() const;
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc index a647994..b9779cc 100644 --- a/quic/core/quic_packet_creator_test.cc +++ b/quic/core/quic_packet_creator_test.cc
@@ -14,6 +14,7 @@ #include "net/third_party/quiche/src/quic/core/crypto/null_encrypter.h" #include "net/third_party/quiche/src/quic/core/crypto/quic_decrypter.h" #include "net/third_party/quiche/src/quic/core/crypto/quic_encrypter.h" +#include "net/third_party/quiche/src/quic/core/frames/quic_stream_frame.h" #include "net/third_party/quiche/src/quic/core/quic_data_writer.h" #include "net/third_party/quiche/src/quic/core/quic_pending_retransmission.h" #include "net/third_party/quiche/src/quic/core/quic_simple_buffer_allocator.h" @@ -21,6 +22,7 @@ #include "net/third_party/quiche/src/quic/core/quic_utils.h" #include "net/third_party/quiche/src/quic/platform/api/quic_arraysize.h" #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_socket_address.h" #include "net/third_party/quiche/src/quic/platform/api/quic_string_piece.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" @@ -79,6 +81,8 @@ ~MockDebugDelegate() override = default; MOCK_METHOD1(OnFrameAddedToPacket, void(const QuicFrame& frame)); + + MOCK_METHOD1(OnStreamFrameCoalesced, void(const QuicStreamFrame& frame)); }; class TestPacketCreator : public QuicPacketCreator { @@ -318,8 +322,10 @@ if (level != ENCRYPTION_INITIAL && level != ENCRYPTION_HANDSHAKE) { frames_.push_back( QuicFrame(QuicStreamFrame(stream_id, false, 0u, QuicStringPiece()))); - frames_.push_back( - QuicFrame(QuicStreamFrame(stream_id, true, 0u, QuicStringPiece()))); + if (!GetQuicReloadableFlag(quic_coalesce_stream_frames)) { + frames_.push_back( + QuicFrame(QuicStreamFrame(stream_id, true, 0u, QuicStringPiece()))); + } } SerializedPacket serialized = SerializeAllFrames(frames_); EXPECT_EQ(level, serialized.encryption_level); @@ -342,7 +348,9 @@ .WillOnce(Return(true)); if (level != ENCRYPTION_INITIAL && level != ENCRYPTION_HANDSHAKE) { EXPECT_CALL(framer_visitor_, OnStreamFrame(_)); - EXPECT_CALL(framer_visitor_, OnStreamFrame(_)); + if (!GetQuicReloadableFlag(quic_coalesce_stream_frames)) { + EXPECT_CALL(framer_visitor_, OnStreamFrame(_)); + } } if (client_framer_.version().HasHeaderProtection()) { EXPECT_CALL(framer_visitor_, OnPaddingFrame(_)) @@ -2334,6 +2342,90 @@ EXPECT_EQ(TestConnectionId(0x33), creator_.GetSourceConnectionId()); } +TEST_P(QuicPacketCreatorTest, CoalesceStreamFrames) { + InSequence s; + if (!GetParam().version_serialization) { + creator_.StopSendingVersion(); + } + SetQuicReloadableFlag(quic_coalesce_stream_frames, true); + const size_t max_plaintext_size = + client_framer_.GetMaxPlaintextSize(creator_.max_packet_length()); + EXPECT_FALSE(creator_.HasPendingFrames()); + creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE); + QuicStreamId stream_id1 = QuicUtils::GetFirstBidirectionalStreamId( + client_framer_.transport_version(), Perspective::IS_CLIENT); + QuicStreamId stream_id2 = GetNthClientInitiatedStreamId(1); + EXPECT_FALSE(creator_.HasPendingStreamFramesOfStream(stream_id1)); + EXPECT_EQ(max_plaintext_size - + GetPacketHeaderSize( + client_framer_.transport_version(), + creator_.GetDestinationConnectionIdLength(), + creator_.GetSourceConnectionIdLength(), + QuicPacketCreatorPeer::SendVersionInPacket(&creator_), + !kIncludeDiversificationNonce, + QuicPacketCreatorPeer::GetPacketNumberLength(&creator_), + QuicPacketCreatorPeer::GetRetryTokenLengthLength(&creator_), + 0, QuicPacketCreatorPeer::GetLengthLength(&creator_)), + creator_.BytesFree()); + StrictMock<MockDebugDelegate> debug; + creator_.set_debug_delegate(&debug); + + MakeIOVector("test", &iov_); + QuicFrame frame; + EXPECT_CALL(debug, OnFrameAddedToPacket(_)); + ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket( + stream_id1, &iov_, 1u, iov_.iov_len, 0u, 0u, false, false, + NOT_RETRANSMISSION, &frame)); + EXPECT_TRUE(creator_.HasPendingFrames()); + EXPECT_TRUE(creator_.HasPendingStreamFramesOfStream(stream_id1)); + + MakeIOVector("coalesce", &iov_); + // frame will be coalesced with the first frame. + const auto previous_size = creator_.PacketSize(); + EXPECT_CALL(debug, OnStreamFrameCoalesced(_)); + ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket( + stream_id1, &iov_, 1u, iov_.iov_len, 0u, 4u, true, false, + NOT_RETRANSMISSION, &frame)); + EXPECT_EQ(frame.stream_frame.data_length, + creator_.PacketSize() - previous_size); + auto queued_frames = QuicPacketCreatorPeer::GetQueuedFrames(&creator_); + EXPECT_EQ(1u, queued_frames.size()); + EXPECT_EQ(12u, queued_frames.front().stream_frame.data_length); + EXPECT_TRUE(queued_frames.front().stream_frame.fin); + + // frame is for another stream, so it won't be coalesced. + const auto length = creator_.BytesFree() - 10u; + std::string large_data("x", length); + MakeIOVector(large_data, &iov_); + EXPECT_CALL(debug, OnFrameAddedToPacket(_)); + ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket( + stream_id2, &iov_, 1u, iov_.iov_len, 0u, 0u, false, false, + NOT_RETRANSMISSION, &frame)); + EXPECT_TRUE(creator_.HasPendingStreamFramesOfStream(stream_id2)); + + // The packet doesn't have enough free bytes for all data, but will still be + // able to consume and coalesce part of them. + EXPECT_CALL(debug, OnStreamFrameCoalesced(_)); + MakeIOVector("somerandomdata", &iov_); + ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket( + stream_id2, &iov_, 1u, iov_.iov_len, 0u, length, false, false, + NOT_RETRANSMISSION, &frame)); + + EXPECT_CALL(delegate_, OnSerializedPacket(_)) + .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket)); + creator_.FlushCurrentPacket(); + EXPECT_CALL(framer_visitor_, OnPacket()); + EXPECT_CALL(framer_visitor_, OnUnauthenticatedPublicHeader(_)); + EXPECT_CALL(framer_visitor_, OnUnauthenticatedHeader(_)); + EXPECT_CALL(framer_visitor_, OnDecryptedPacket(_)); + EXPECT_CALL(framer_visitor_, OnPacketHeader(_)); + // The packet should only have 2 stream frames. + EXPECT_CALL(framer_visitor_, OnStreamFrame(_)); + EXPECT_CALL(framer_visitor_, OnStreamFrame(_)); + EXPECT_CALL(framer_visitor_, OnPacketComplete()); + ProcessPacket(serialized_packet_); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_packet_generator_test.cc b/quic/core/quic_packet_generator_test.cc index b098958..5749ce0 100644 --- a/quic/core/quic_packet_generator_test.cc +++ b/quic/core/quic_packet_generator_test.cc
@@ -600,14 +600,14 @@ generator_.ConsumeData( QuicUtils::GetFirstBidirectionalStreamId(framer_.transport_version(), Perspective::IS_CLIENT), - &iov_, 1u, iov_.iov_len, 0, FIN); + &iov_, 1u, iov_.iov_len, 0, NO_FIN); MakeIOVector("quux", &iov_); QuicConsumedData consumed = generator_.ConsumeData( QuicUtils::GetFirstBidirectionalStreamId(framer_.transport_version(), Perspective::IS_CLIENT), - &iov_, 1u, iov_.iov_len, 3, NO_FIN); + &iov_, 1u, iov_.iov_len, 3, FIN); EXPECT_EQ(4u, consumed.bytes_consumed); - EXPECT_FALSE(consumed.fin_consumed); + EXPECT_TRUE(consumed.fin_consumed); EXPECT_TRUE(generator_.HasPendingFrames()); EXPECT_TRUE(generator_.HasRetransmittableFrames()); @@ -619,7 +619,8 @@ EXPECT_FALSE(generator_.HasRetransmittableFrames()); PacketContents contents; - contents.num_stream_frames = 2; + contents.num_stream_frames = + GetQuicReloadableFlag(quic_coalesce_stream_frames) ? 1 : 2; CheckPacketContains(contents, 0); }
diff --git a/quic/test_tools/quic_packet_creator_peer.cc b/quic/test_tools/quic_packet_creator_peer.cc index d4e5598..20a3834 100644 --- a/quic/test_tools/quic_packet_creator_peer.cc +++ b/quic/test_tools/quic_packet_creator_peer.cc
@@ -4,6 +4,7 @@ #include "net/third_party/quiche/src/quic/test_tools/quic_packet_creator_peer.h" +#include "net/third_party/quiche/src/quic/core/frames/quic_frame.h" #include "net/third_party/quiche/src/quic/core/quic_packet_creator.h" #include "net/third_party/quiche/src/quic/core/quic_types.h" @@ -57,6 +58,12 @@ return creator->GetLengthLength(); } +// static +const QuicFrames& QuicPacketCreatorPeer::GetQueuedFrames( + QuicPacketCreator* creator) { + return creator->queued_frames_; +} + void QuicPacketCreatorPeer::SetPacketNumber(QuicPacketCreator* creator, uint64_t s) { DCHECK_NE(0u, s);
diff --git a/quic/test_tools/quic_packet_creator_peer.h b/quic/test_tools/quic_packet_creator_peer.h index e040090..a633ef4 100644 --- a/quic/test_tools/quic_packet_creator_peer.h +++ b/quic/test_tools/quic_packet_creator_peer.h
@@ -26,6 +26,7 @@ QuicPacketNumberLength packet_number_length); static QuicPacketNumberLength GetPacketNumberLength( QuicPacketCreator* creator); + static const QuicFrames& GetQueuedFrames(QuicPacketCreator* creator); static QuicVariableLengthIntegerLength GetRetryTokenLengthLength( QuicPacketCreator* creator); static QuicVariableLengthIntegerLength GetLengthLength(