Coalesce QPACK encoder stream writes in QpackEncoderStreamSender.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 273553497
Change-Id: Ia68206d3ff10995e722e2ba87c84e7047bb418f4
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc
index a3420be..eac158b 100644
--- a/quic/core/qpack/qpack_encoder.cc
+++ b/quic/core/qpack/qpack_encoder.cc
@@ -281,6 +281,8 @@
}
}
+ encoder_stream_sender_.Flush();
+
// Use local |sent_byte_count| variable to avoid branching and dereferencing
// each time encoder stream data is sent.
if (encoder_stream_sent_byte_count) {
@@ -395,6 +397,8 @@
void QpackEncoder::SetDynamicTableCapacity(uint64_t dynamic_table_capacity) {
encoder_stream_sender_.SendSetDynamicTableCapacity(dynamic_table_capacity);
+ encoder_stream_sender_.Flush();
+
bool success = header_table_.SetDynamicTableCapacity(dynamic_table_capacity);
DCHECK(success);
}
diff --git a/quic/core/qpack/qpack_encoder.h b/quic/core/qpack/qpack_encoder.h
index be4f4e2..1ed56a8 100644
--- a/quic/core/qpack/qpack_encoder.h
+++ b/quic/core/qpack/qpack_encoder.h
@@ -124,10 +124,10 @@
// Performs first pass of two-pass encoding: represent each header field in
// |*header_list| as a reference to an existing entry, the name of an existing
// entry with a literal value, or a literal name and value pair. Sends
- // necessary instructions on the encoder stream. Records absolute indices of
- // referred dynamic table entries in |*referred_indices|. If
- // |encoder_stream_sent_byte_count| is not null, then sets
- // |*encoder_stream_sent_byte_count| to the number of bytes sent on the
+ // necessary instructions on the encoder stream coalesced in a single write.
+ // Records absolute indices of referred dynamic table entries in
+ // |*referred_indices|. If |encoder_stream_sent_byte_count| is not null, then
+ // sets |*encoder_stream_sent_byte_count| to the number of bytes sent on the
// encoder stream to insert dynamic table entries. Returns list of header
// field representations, with all dynamic table entries referred to with
// absolute indices. Returned Instructions object may have QuicStringPieces
diff --git a/quic/core/qpack/qpack_encoder_stream_sender.cc b/quic/core/qpack/qpack_encoder_stream_sender.cc
index 533e3a1..8df0b03 100644
--- a/quic/core/qpack/qpack_encoder_stream_sender.cc
+++ b/quic/core/qpack/qpack_encoder_stream_sender.cc
@@ -23,11 +23,7 @@
values_.varint = name_index;
values_.value = value;
- std::string output;
- instruction_encoder_.Encode(InsertWithNameReferenceInstruction(), values_,
- &output);
- delegate_->WriteStreamData(output);
- return output.size();
+ return Encode(InsertWithNameReferenceInstruction());
}
QuicByteCount QpackEncoderStreamSender::SendInsertWithoutNameReference(
@@ -36,31 +32,36 @@
values_.name = name;
values_.value = value;
- std::string output;
- instruction_encoder_.Encode(InsertWithoutNameReferenceInstruction(), values_,
- &output);
- delegate_->WriteStreamData(output);
- return output.size();
+ return Encode(InsertWithoutNameReferenceInstruction());
}
QuicByteCount QpackEncoderStreamSender::SendDuplicate(uint64_t index) {
values_.varint = index;
- std::string output;
- instruction_encoder_.Encode(DuplicateInstruction(), values_, &output);
- delegate_->WriteStreamData(output);
- return output.size();
+ return Encode(DuplicateInstruction());
}
QuicByteCount QpackEncoderStreamSender::SendSetDynamicTableCapacity(
uint64_t capacity) {
values_.varint = capacity;
- std::string output;
- instruction_encoder_.Encode(SetDynamicTableCapacityInstruction(), values_,
- &output);
- delegate_->WriteStreamData(output);
- return output.size();
+ return Encode(SetDynamicTableCapacityInstruction());
+}
+
+void QpackEncoderStreamSender::Flush() {
+ if (buffer_.empty()) {
+ return;
+ }
+
+ delegate_->WriteStreamData(buffer_);
+ buffer_.clear();
+}
+
+QuicByteCount QpackEncoderStreamSender::Encode(
+ const QpackInstruction* instruction) {
+ const size_t old_buffer_size = buffer_.size();
+ instruction_encoder_.Encode(instruction, values_, &buffer_);
+ return buffer_.size() - old_buffer_size;
}
} // namespace quic
diff --git a/quic/core/qpack/qpack_encoder_stream_sender.h b/quic/core/qpack/qpack_encoder_stream_sender.h
index c8c7717..5c4af7d 100644
--- a/quic/core/qpack/qpack_encoder_stream_sender.h
+++ b/quic/core/qpack/qpack_encoder_stream_sender.h
@@ -16,13 +16,14 @@
namespace quic {
// This class serializes instructions for transmission on the encoder stream.
+// Serialized instructions are buffered until Flush() is called.
class QUIC_EXPORT_PRIVATE QpackEncoderStreamSender {
public:
QpackEncoderStreamSender();
QpackEncoderStreamSender(const QpackEncoderStreamSender&) = delete;
QpackEncoderStreamSender& operator=(const QpackEncoderStreamSender&) = delete;
- // Methods for sending instructions, see
+ // Methods for serializing and buffering instructions, see
// https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#rfc.section.5.2
// 5.2.1. Insert With Name Reference
@@ -37,15 +38,23 @@
// 5.2.4. Set Dynamic Table Capacity
QuicByteCount SendSetDynamicTableCapacity(uint64_t capacity);
+ // Writes all buffered instructions on the encoder stream.
+ void Flush();
+
// delegate must be set if dynamic table capacity is not zero.
void set_qpack_stream_sender_delegate(QpackStreamSenderDelegate* delegate) {
delegate_ = delegate;
}
private:
+ // Encodes |instruction| with |values_| using |instruction_encoder_| into
+ // |buffer_|. Returns number of bytes encoded.
+ QuicByteCount Encode(const QpackInstruction* instruction);
+
QpackStreamSenderDelegate* delegate_;
QpackInstructionEncoder instruction_encoder_;
QpackInstructionEncoder::Values values_;
+ std::string buffer_;
};
} // namespace quic
diff --git a/quic/core/qpack/qpack_encoder_stream_sender_test.cc b/quic/core/qpack/qpack_encoder_stream_sender_test.cc
index dcfd5bd..b1ddd97 100644
--- a/quic/core/qpack/qpack_encoder_stream_sender_test.cc
+++ b/quic/core/qpack/qpack_encoder_stream_sender_test.cc
@@ -32,18 +32,21 @@
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendInsertWithNameReference(true, 5, ""));
+ stream_.Flush();
// Static, index fits in prefix, Huffman encoded value.
expected_encoded_data = QuicTextUtils::HexDecode("c28294e7");
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendInsertWithNameReference(true, 2, "foo"));
+ stream_.Flush();
// Not static, index does not fit in prefix, not Huffman encoded value.
expected_encoded_data = QuicTextUtils::HexDecode("bf4a03626172");
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendInsertWithNameReference(false, 137, "bar"));
+ stream_.Flush();
// Value length does not fit in prefix.
// 'Z' would be Huffman encoded to 8 bits, so no Huffman encoding is used.
@@ -56,6 +59,7 @@
EXPECT_EQ(
expected_encoded_data.size(),
stream_.SendInsertWithNameReference(false, 42, std::string(127, 'Z')));
+ stream_.Flush();
}
TEST_F(QpackEncoderStreamSenderTest, InsertWithoutNameReference) {
@@ -64,18 +68,21 @@
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendInsertWithoutNameReference("", ""));
+ stream_.Flush();
// Huffman encoded short strings.
- expected_encoded_data = QuicTextUtils::HexDecode("4362617203626172");
- EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
- EXPECT_EQ(expected_encoded_data.size(),
- stream_.SendInsertWithoutNameReference("bar", "bar"));
-
- // Not Huffman encoded short strings.
expected_encoded_data = QuicTextUtils::HexDecode("6294e78294e7");
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendInsertWithoutNameReference("foo", "foo"));
+ stream_.Flush();
+
+ // Not Huffman encoded short strings.
+ expected_encoded_data = QuicTextUtils::HexDecode("4362617203626172");
+ EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
+ EXPECT_EQ(expected_encoded_data.size(),
+ stream_.SendInsertWithoutNameReference("bar", "bar"));
+ 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.
@@ -89,6 +96,7 @@
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendInsertWithoutNameReference(std::string(31, 'Z'),
std::string(127, 'Z')));
+ stream_.Flush();
}
TEST_F(QpackEncoderStreamSenderTest, Duplicate) {
@@ -96,11 +104,13 @@
std::string expected_encoded_data = QuicTextUtils::HexDecode("11");
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(), stream_.SendDuplicate(17));
+ stream_.Flush();
// Large index requires two extension bytes.
expected_encoded_data = QuicTextUtils::HexDecode("1fd503");
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(), stream_.SendDuplicate(500));
+ stream_.Flush();
}
TEST_F(QpackEncoderStreamSenderTest, SetDynamicTableCapacity) {
@@ -109,12 +119,48 @@
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendSetDynamicTableCapacity(17));
+ stream_.Flush();
// Large capacity requires two extension bytes.
expected_encoded_data = QuicTextUtils::HexDecode("3fd503");
EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data)));
EXPECT_EQ(expected_encoded_data.size(),
stream_.SendSetDynamicTableCapacity(500));
+ stream_.Flush();
+}
+
+// No writes should happen until Flush is called.
+TEST_F(QpackEncoderStreamSenderTest, Coalesce) {
+ // Insert entry with static name reference, empty value.
+ std::string expected_encoded_data1 = QuicTextUtils::HexDecode("c500");
+ EXPECT_EQ(expected_encoded_data1.size(),
+ stream_.SendInsertWithNameReference(true, 5, ""));
+
+ // Insert entry with static name reference, Huffman encoded value.
+ std::string expected_encoded_data2 = QuicTextUtils::HexDecode("c28294e7");
+ EXPECT_EQ(expected_encoded_data2.size(),
+ stream_.SendInsertWithNameReference(true, 2, "foo"));
+
+ // Insert literal entry, Huffman encoded short strings.
+ std::string expected_encoded_data3 = QuicTextUtils::HexDecode("6294e78294e7");
+ EXPECT_EQ(expected_encoded_data3.size(),
+ stream_.SendInsertWithoutNameReference("foo", "foo"));
+
+ // Duplicate entry.
+ std::string expected_encoded_data4 = QuicTextUtils::HexDecode("11");
+ EXPECT_EQ(expected_encoded_data4.size(), stream_.SendDuplicate(17));
+
+ EXPECT_CALL(delegate_,
+ WriteStreamData(Eq(QuicStrCat(
+ expected_encoded_data1 + expected_encoded_data2 +
+ expected_encoded_data3 + expected_encoded_data4))));
+ stream_.Flush();
+}
+
+// No writes should happen if QpackEncoderStreamSender::Flush() is called
+// when the buffer is empty.
+TEST_F(QpackEncoderStreamSenderTest, FlushEmpty) {
+ stream_.Flush();
}
} // namespace
diff --git a/quic/core/qpack/qpack_encoder_test.cc b/quic/core/qpack/qpack_encoder_test.cc
index 9850822..3bc5859 100644
--- a/quic/core/qpack/qpack_encoder_test.cc
+++ b/quic/core/qpack/qpack_encoder_test.cc
@@ -202,32 +202,23 @@
header_list["cookie"] = "baz"; // name matches static entry
// Insert three entries into the dynamic table.
- std::string insert_entry1 = QuicTextUtils::HexDecode(
+ std::string insert_entries = QuicTextUtils::HexDecode(
"62" // insert without name reference
"94e7" // Huffman-encoded name "foo"
- "03626172"); // value "bar"
- EXPECT_CALL(encoder_stream_sender_delegate_,
- WriteStreamData(Eq(insert_entry1)));
-
- std::string insert_entry2 = QuicTextUtils::HexDecode(
+ "03626172" // value "bar"
"80" // insert with name reference, dynamic index 0
- "0362617a"); // value "baz"
- EXPECT_CALL(encoder_stream_sender_delegate_,
- WriteStreamData(Eq(insert_entry2)));
-
- std::string insert_entry3 = QuicTextUtils::HexDecode(
+ "0362617a" // value "baz"
"c5" // insert with name reference, static index 5
"0362617a"); // value "baz"
EXPECT_CALL(encoder_stream_sender_delegate_,
- WriteStreamData(Eq(insert_entry3)));
+ WriteStreamData(Eq(insert_entries)));
EXPECT_EQ(QuicTextUtils::HexDecode(
"0400" // prefix
"828180"), // dynamic entries with relative index 0, 1, and 2
Encode(header_list));
- EXPECT_EQ(insert_entry1.size() + insert_entry2.size() + insert_entry3.size(),
- encoder_stream_sent_byte_count_);
+ EXPECT_EQ(insert_entries.size(), encoder_stream_sent_byte_count_);
}
// There is no room in the dynamic table after inserting the first entry.
@@ -320,31 +311,22 @@
encoder_.OnInsertCountIncrement(1);
// Insert three entries into the dynamic table.
- std::string insert_entry2 = QuicTextUtils::HexDecode(
+ std::string insert_entries = QuicTextUtils::HexDecode(
"80" // insert with name reference, dynamic index 0
- "0362617a"); // value "baz"
- EXPECT_CALL(encoder_stream_sender_delegate_,
- WriteStreamData(Eq(insert_entry2)));
-
- std::string insert_entry3 = QuicTextUtils::HexDecode(
+ "0362617a" // value "baz"
"c5" // insert with name reference, static index 5
- "0362617a"); // value "baz"
- EXPECT_CALL(encoder_stream_sender_delegate_,
- WriteStreamData(Eq(insert_entry3)));
-
- std::string insert_entry4 = QuicTextUtils::HexDecode(
+ "0362617a" // value "baz"
"43" // insert without name reference
"626172" // name "bar"
"0362617a"); // value "baz"
EXPECT_CALL(encoder_stream_sender_delegate_,
- WriteStreamData(Eq(insert_entry4)));
+ WriteStreamData(Eq(insert_entries)));
EXPECT_EQ(QuicTextUtils::HexDecode("0500" // prefix
"83828180"), // dynamic entries
encoder_.EncodeHeaderList(/* stream_id = */ 3, header_list2,
&encoder_stream_sent_byte_count_));
- EXPECT_EQ(insert_entry2.size() + insert_entry3.size() + insert_entry4.size(),
- encoder_stream_sent_byte_count_);
+ EXPECT_EQ(insert_entries.size(), encoder_stream_sent_byte_count_);
// Stream 3 is blocked. Stream 4 is not allowed to block, but it can
// reference already acknowledged dynamic entry 0.
@@ -414,7 +396,7 @@
encoder_.SetDynamicTableCapacity(maximum_dynamic_table_capacity);
// Insert ten entries into the dynamic table.
- EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_)).Times(10);
+ EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_));
EXPECT_EQ(
QuicTextUtils::HexDecode("0b00" // prefix
