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(
