gfe-relnote: Delay writes on QPACK streams. Protected by gfe2_reloadable_flag_quic_enable_version_draft_25_v3 and gfe2_reloadable_flag_quic_enable_version_draft_27. PiperOrigin-RevId: 304827535 Change-Id: Ib21ef08783568db3150845b34b4341e8523ad950
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 3dd9be3..e16fede 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -740,8 +740,6 @@ SendMaxPushId(); http3_max_push_id_sent_ = true; } - qpack_decoder_send_stream_->MaybeSendStreamType(); - qpack_encoder_send_stream_->MaybeSendStreamType(); } QpackEncoder* QuicSpdySession::qpack_encoder() {
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index 2007806..49d9697 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -2418,8 +2418,6 @@ EXPECT_NE(5u, session_.max_outbound_header_list_size()); EXPECT_NE(42u, QpackEncoderPeer::maximum_blocked_streams(qpack_encoder)); - EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _)) - .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0))); EXPECT_CALL(debug_visitor, OnSettingsFrameReceived(settings)); session_.OnStreamFrame(frame);
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index fab6d86..4f9aefa 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -337,14 +337,6 @@ EXPECT_CALL(*session_, WritevData(send_control_stream->id(), _, _, _, _, _)) .Times(num_control_stream_writes); - auto qpack_decoder_stream = - QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - EXPECT_CALL(*session_, - WritevData(qpack_decoder_stream->id(), 1, 0, _, _, _)); - auto qpack_encoder_stream = - QuicSpdySessionPeer::GetQpackEncoderSendStream(session_.get()); - EXPECT_CALL(*session_, - WritevData(qpack_encoder_stream->id(), 1, 0, _, _, _)); } TestCryptoStream* crypto_stream = session_->GetMutableCryptoStream(); EXPECT_CALL(*crypto_stream, HasPendingRetransmission()) @@ -2148,8 +2140,14 @@ std::string headers = HeadersFrame(encoded_headers); EXPECT_CALL(debug_visitor, OnHeadersFrameReceived(stream_->id(), encoded_headers.length())); - // Decoder stream type and header acknowledgement. - EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _)); + // Decoder stream type. + EXPECT_CALL(*session_, + WritevData(decoder_send_stream->id(), /* write_length = */ 1, + /* offset = */ 0, _, _, _)); + // Header acknowledgement. + EXPECT_CALL(*session_, + WritevData(decoder_send_stream->id(), /* write_length = */ 1, + /* offset = */ 1, _, _, _)); EXPECT_CALL(debug_visitor, OnHeadersDecoded(stream_->id(), _)); stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, 0, headers)); @@ -2216,8 +2214,14 @@ auto decoder_send_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - // Decoder stream type and header acknowledgement. - EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _)); + // Decoder stream type. + EXPECT_CALL(*session_, + WritevData(decoder_send_stream->id(), /* write_length = */ 1, + /* offset = */ 0, _, _, _)); + // Header acknowledgement. + EXPECT_CALL(*session_, + WritevData(decoder_send_stream->id(), /* write_length = */ 1, + /* offset = */ 1, _, _, _)); EXPECT_CALL(debug_visitor, OnHeadersDecoded(stream_->id(), _)); // Deliver dynamic table entry to decoder. session_->qpack_decoder()->OnInsertWithoutNameReference("foo", "bar"); @@ -2247,6 +2251,7 @@ // Decoding is blocked because dynamic table entry has not been received yet. EXPECT_FALSE(stream_->trailers_decompressed()); + // Header acknowledgement. EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _)); EXPECT_CALL(debug_visitor, OnHeadersDecoded(stream_->id(), _)); // Deliver second dynamic table entry to decoder. @@ -2341,8 +2346,14 @@ auto decoder_send_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - // The stream byte will be written in the first byte. - EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _)); + // Decoder stream type. + EXPECT_CALL(*session_, + WritevData(decoder_send_stream->id(), /* write_length = */ 1, + /* offset = */ 0, _, _, _)); + // Header acknowledgement. + EXPECT_CALL(*session_, + WritevData(decoder_send_stream->id(), /* write_length = */ 1, + /* offset = */ 1, _, _, _)); // Deliver dynamic table entry to decoder. session_->qpack_decoder()->OnInsertWithoutNameReference("foo", "bar"); EXPECT_TRUE(stream_->headers_decompressed()); @@ -2794,7 +2805,14 @@ auto qpack_decoder_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - EXPECT_CALL(*session_, WritevData(qpack_decoder_stream->id(), 1, 1, _, _, _)); + // Stream type. + EXPECT_CALL(*session_, + WritevData(qpack_decoder_stream->id(), /* write_length = */ 1, + /* offset = */ 0, _, _, _)); + // Stream cancellation. + EXPECT_CALL(*session_, + WritevData(qpack_decoder_stream->id(), /* write_length = */ 1, + /* offset = */ 1, _, _, _)); EXPECT_CALL(*session_, SendRstStream(stream_->id(), QUIC_STREAM_CANCELLED, 0)); @@ -2812,7 +2830,14 @@ auto qpack_decoder_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - EXPECT_CALL(*session_, WritevData(qpack_decoder_stream->id(), 1, 1, _, _, _)); + // Stream type. + EXPECT_CALL(*session_, + WritevData(qpack_decoder_stream->id(), /* write_length = */ 1, + /* offset = */ 0, _, _, _)); + // Stream cancellation. + EXPECT_CALL(*session_, + WritevData(qpack_decoder_stream->id(), /* write_length = */ 1, + /* offset = */ 1, _, _, _)); stream_->OnStreamReset(QuicRstStreamFrame( kInvalidControlFrameId, stream_->id(), QUIC_STREAM_CANCELLED, 0));
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc index 2b30e98..db7ec79 100644 --- a/quic/core/qpack/qpack_encoder.cc +++ b/quic/core/qpack/qpack_encoder.cc
@@ -80,6 +80,11 @@ const spdy::SpdyHeaderBlock& header_list, QpackBlockingManager::IndexSet* referred_indices, QuicByteCount* encoder_stream_sent_byte_count) { + // If previous instructions are buffered in |encoder_stream_sender_|, + // do not count them towards the current header block. + const QuicByteCount initial_encoder_stream_buffered_byte_count = + encoder_stream_sender_.BufferedByteCount(); + Instructions instructions; instructions.reserve(header_list.size()); @@ -266,10 +271,16 @@ } } - const QuicByteCount sent_byte_count = encoder_stream_sender_.Flush(); + const QuicByteCount encoder_stream_buffered_byte_count = + encoder_stream_sender_.BufferedByteCount(); + DCHECK_GE(encoder_stream_buffered_byte_count, + initial_encoder_stream_buffered_byte_count); if (encoder_stream_sent_byte_count) { - *encoder_stream_sent_byte_count = sent_byte_count; + *encoder_stream_sent_byte_count = + encoder_stream_buffered_byte_count - + initial_encoder_stream_buffered_byte_count; } + encoder_stream_sender_.Flush(); ++header_list_count_; @@ -374,7 +385,8 @@ void QpackEncoder::SetDynamicTableCapacity(uint64_t dynamic_table_capacity) { encoder_stream_sender_.SendSetDynamicTableCapacity(dynamic_table_capacity); - encoder_stream_sender_.Flush(); + // Do not flush encoder stream. This write can safely be delayed until more + // instructions are written. bool success = header_table_.SetDynamicTableCapacity(dynamic_table_capacity); DCHECK(success);
diff --git a/quic/core/qpack/qpack_encoder_stream_sender.cc b/quic/core/qpack/qpack_encoder_stream_sender.cc index 19311b6..0ae1aae 100644 --- a/quic/core/qpack/qpack_encoder_stream_sender.cc +++ b/quic/core/qpack/qpack_encoder_stream_sender.cc
@@ -44,15 +44,13 @@ QpackInstructionWithValues::SetDynamicTableCapacity(capacity), &buffer_); } -QuicByteCount QpackEncoderStreamSender::Flush() { +void QpackEncoderStreamSender::Flush() { if (buffer_.empty()) { - return 0; + return; } delegate_->WriteStreamData(buffer_); - const QuicByteCount bytes_written = buffer_.size(); buffer_.clear(); - return bytes_written; } } // namespace quic
diff --git a/quic/core/qpack/qpack_encoder_stream_sender.h b/quic/core/qpack/qpack_encoder_stream_sender.h index 5701db5..dee23c5 100644 --- a/quic/core/qpack/qpack_encoder_stream_sender.h +++ b/quic/core/qpack/qpack_encoder_stream_sender.h
@@ -38,9 +38,11 @@ // 5.2.4. Set Dynamic Table Capacity void SendSetDynamicTableCapacity(uint64_t capacity); + // Returns number of buffered bytes. + QuicByteCount BufferedByteCount() const { return buffer_.size(); } + // Writes all buffered instructions on the encoder stream. - // Returns the number of bytes written. - QuicByteCount Flush(); + void Flush(); // delegate must be set if dynamic table capacity is not zero. void set_qpack_stream_sender_delegate(QpackStreamSenderDelegate* delegate) {
diff --git a/quic/core/qpack/qpack_encoder_stream_sender_test.cc b/quic/core/qpack/qpack_encoder_stream_sender_test.cc index b3fd5f9..ea45acc 100644 --- a/quic/core/qpack/qpack_encoder_stream_sender_test.cc +++ b/quic/core/qpack/qpack_encoder_stream_sender_test.cc
@@ -27,24 +27,29 @@ }; TEST_F(QpackEncoderStreamSenderTest, InsertWithNameReference) { + EXPECT_EQ(0u, stream_.BufferedByteCount()); + // Static, index fits in prefix, empty value. std::string expected_encoded_data = quiche::QuicheTextUtils::HexDecode("c500"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithNameReference(true, 5, ""); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); // Static, index fits in prefix, Huffman encoded value. expected_encoded_data = quiche::QuicheTextUtils::HexDecode("c28294e7"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithNameReference(true, 2, "foo"); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); // Not static, index does not fit in prefix, not Huffman encoded value. expected_encoded_data = quiche::QuicheTextUtils::HexDecode("bf4a03626172"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithNameReference(false, 137, "bar"); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); // Value length does not fit in prefix. // 'Z' would be Huffman encoded to 8 bits, so no Huffman encoding is used. @@ -55,29 +60,35 @@ "5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithNameReference(false, 42, std::string(127, 'Z')); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); } TEST_F(QpackEncoderStreamSenderTest, InsertWithoutNameReference) { + EXPECT_EQ(0u, stream_.BufferedByteCount()); + // Empty name and value. std::string expected_encoded_data = quiche::QuicheTextUtils::HexDecode("4000"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithoutNameReference("", ""); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); // Huffman encoded short strings. expected_encoded_data = quiche::QuicheTextUtils::HexDecode("6294e78294e7"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithoutNameReference("foo", "foo"); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); // Not Huffman encoded short strings. expected_encoded_data = quiche::QuicheTextUtils::HexDecode("4362617203626172"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithoutNameReference("bar", "bar"); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); // Not Huffman encoded long strings; length does not fit on prefix. // 'Z' would be Huffman encoded to 8 bits, so no Huffman encoding is used. @@ -90,35 +101,46 @@ EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendInsertWithoutNameReference(std::string(31, 'Z'), std::string(127, 'Z')); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); } TEST_F(QpackEncoderStreamSenderTest, Duplicate) { + EXPECT_EQ(0u, stream_.BufferedByteCount()); + // Small index fits in prefix. std::string expected_encoded_data = quiche::QuicheTextUtils::HexDecode("11"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendDuplicate(17); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); // Large index requires two extension bytes. expected_encoded_data = quiche::QuicheTextUtils::HexDecode("1fd503"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendDuplicate(500); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); } TEST_F(QpackEncoderStreamSenderTest, SetDynamicTableCapacity) { + EXPECT_EQ(0u, stream_.BufferedByteCount()); + // Small capacity fits in prefix. std::string expected_encoded_data = quiche::QuicheTextUtils::HexDecode("31"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendSetDynamicTableCapacity(17); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); + EXPECT_EQ(0u, stream_.BufferedByteCount()); // Large capacity requires two extension bytes. expected_encoded_data = quiche::QuicheTextUtils::HexDecode("3fd503"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); stream_.SendSetDynamicTableCapacity(500); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); + EXPECT_EQ(0u, stream_.BufferedByteCount()); } // No writes should happen until Flush is called. @@ -142,13 +164,17 @@ "11"); // Duplicate entry. EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); - EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); + EXPECT_EQ(expected_encoded_data.size(), stream_.BufferedByteCount()); + stream_.Flush(); + EXPECT_EQ(0u, stream_.BufferedByteCount()); } // No writes should happen if QpackEncoderStreamSender::Flush() is called // when the buffer is empty. TEST_F(QpackEncoderStreamSenderTest, FlushEmpty) { - EXPECT_EQ(0u, stream_.Flush()); + EXPECT_EQ(0u, stream_.BufferedByteCount()); + stream_.Flush(); + EXPECT_EQ(0u, stream_.BufferedByteCount()); } } // namespace
diff --git a/quic/core/qpack/qpack_encoder_test.cc b/quic/core/qpack/qpack_encoder_test.cc index 212257c..e0d21c5 100644 --- a/quic/core/qpack/qpack_encoder_test.cc +++ b/quic/core/qpack/qpack_encoder_test.cc
@@ -12,6 +12,7 @@ #include "net/third_party/quiche/src/quic/test_tools/qpack/qpack_encoder_test_utils.h" #include "net/third_party/quiche/src/quic/test_tools/qpack/qpack_header_table_peer.h" #include "net/third_party/quiche/src/quic/test_tools/qpack/qpack_test_utils.h" +#include "net/third_party/quiche/src/common/platform/api/quiche_str_cat.h" #include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h" #include "net/third_party/quiche/src/common/platform/api/quiche_text_utils.h" @@ -213,11 +214,6 @@ TEST_F(QpackEncoderTest, DynamicTable) { encoder_.SetMaximumBlockedStreams(1); encoder_.SetMaximumDynamicTableCapacity(4096); - - // Set Dynamic Table Capacity instruction. - EXPECT_CALL( - encoder_stream_sender_delegate_, - WriteStreamData(Eq(quiche::QuicheTextUtils::HexDecode("3fe11f")))); encoder_.SetDynamicTableCapacity(4096); spdy::SpdyHeaderBlock header_list; @@ -226,6 +222,9 @@ "baz"); // name matches dynamic entry header_list["cookie"] = "baz"; // name matches static entry + // Set Dynamic Table Capacity instruction. + std::string set_dyanamic_table_capacity = + quiche::QuicheTextUtils::HexDecode("3fe11f"); // Insert three entries into the dynamic table. std::string insert_entries = quiche::QuicheTextUtils::HexDecode( "62" // insert without name reference @@ -236,7 +235,8 @@ "c5" // insert with name reference, static index 5 "0362617a"); // value "baz" EXPECT_CALL(encoder_stream_sender_delegate_, - WriteStreamData(Eq(insert_entries))); + WriteStreamData(Eq(quiche::QuicheStrCat( + set_dyanamic_table_capacity, insert_entries)))); EXPECT_EQ(quiche::QuicheTextUtils::HexDecode( "0400" // prefix @@ -250,10 +250,6 @@ TEST_F(QpackEncoderTest, SmallDynamicTable) { encoder_.SetMaximumBlockedStreams(1); encoder_.SetMaximumDynamicTableCapacity(QpackEntry::Size("foo", "bar")); - - // Set Dynamic Table Capacity instruction. - EXPECT_CALL(encoder_stream_sender_delegate_, - WriteStreamData(Eq(quiche::QuicheTextUtils::HexDecode("3f07")))); encoder_.SetDynamicTableCapacity(QpackEntry::Size("foo", "bar")); spdy::SpdyHeaderBlock header_list; @@ -263,13 +259,17 @@ header_list["cookie"] = "baz"; // name matches static entry header_list["bar"] = "baz"; // no match + // Set Dynamic Table Capacity instruction. + std::string set_dyanamic_table_capacity = + quiche::QuicheTextUtils::HexDecode("3f07"); // Insert one entry into the dynamic table. std::string insert_entry = quiche::QuicheTextUtils::HexDecode( "62" // insert without name reference "94e7" // Huffman-encoded name "foo" "03626172"); // value "bar" EXPECT_CALL(encoder_stream_sender_delegate_, - WriteStreamData(Eq(insert_entry))); + WriteStreamData(Eq(quiche::QuicheStrCat( + set_dyanamic_table_capacity, insert_entry)))); EXPECT_EQ(quiche::QuicheTextUtils::HexDecode( "0200" // prefix @@ -288,23 +288,22 @@ TEST_F(QpackEncoderTest, BlockedStream) { encoder_.SetMaximumBlockedStreams(1); encoder_.SetMaximumDynamicTableCapacity(4096); - - // Set Dynamic Table Capacity instruction. - EXPECT_CALL( - encoder_stream_sender_delegate_, - WriteStreamData(Eq(quiche::QuicheTextUtils::HexDecode("3fe11f")))); encoder_.SetDynamicTableCapacity(4096); spdy::SpdyHeaderBlock header_list1; header_list1["foo"] = "bar"; + // Set Dynamic Table Capacity instruction. + std::string set_dyanamic_table_capacity = + quiche::QuicheTextUtils::HexDecode("3fe11f"); // Insert one entry into the dynamic table. std::string insert_entry1 = quiche::QuicheTextUtils::HexDecode( "62" // insert without name reference "94e7" // Huffman-encoded name "foo" "03626172"); // value "bar" EXPECT_CALL(encoder_stream_sender_delegate_, - WriteStreamData(Eq(insert_entry1))); + WriteStreamData(Eq(quiche::QuicheStrCat( + set_dyanamic_table_capacity, insert_entry1)))); EXPECT_EQ(quiche::QuicheTextUtils::HexDecode("0200" // prefix "80"), // dynamic entry 0 @@ -420,12 +419,10 @@ } maximum_dynamic_table_capacity += QpackEntry::Size("one", "foo"); encoder_.SetMaximumDynamicTableCapacity(maximum_dynamic_table_capacity); - - // Set Dynamic Table Capacity instruction. - EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_)); encoder_.SetDynamicTableCapacity(maximum_dynamic_table_capacity); - // Insert ten entries into the dynamic table. + // Set Dynamic Table Capacity instruction and insert ten entries into the + // dynamic table. EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_)); EXPECT_EQ(quiche::QuicheTextUtils::HexDecode( @@ -466,10 +463,6 @@ TEST_F(QpackEncoderTest, DynamicTableCapacityLessThanMaximum) { encoder_.SetMaximumDynamicTableCapacity(1024); - - // Set Dynamic Table Capacity instruction. - EXPECT_CALL(encoder_stream_sender_delegate_, - WriteStreamData(Eq(quiche::QuicheTextUtils::HexDecode("3e")))); encoder_.SetDynamicTableCapacity(30); QpackHeaderTable* header_table = QpackEncoderPeer::header_table(&encoder_);