Simplify QpackEncoderStreamSender API. Instead of each Send*() method returning the number of bytes encoded, return the total number of bytes written from Flush(). This is a follow-up to cr/273553497. gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 273775165 Change-Id: Ifaf1b55d6d0ab26cc3ab23686bcb7427e3c9627f
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc index eac158b..b1f7fbe 100644 --- a/quic/core/qpack/qpack_encoder.cc +++ b/quic/core/qpack/qpack_encoder.cc
@@ -93,7 +93,6 @@ QuicByteCount* encoder_stream_sent_byte_count) { Instructions instructions; instructions.reserve(header_list.size()); - QuicByteCount sent_byte_count = 0; // The index of the oldest entry that must not be evicted. uint64_t smallest_blocking_index = @@ -157,7 +156,7 @@ dynamic_table_insertion_blocked = true; } else { // If allowed, duplicate entry and refer to it. - sent_byte_count += encoder_stream_sender_.SendDuplicate( + encoder_stream_sender_.SendDuplicate( QpackAbsoluteIndexToEncoderStreamRelativeIndex( index, header_table_.inserted_entry_count())); auto entry = header_table_.InsertEntry(name, value); @@ -187,9 +186,8 @@ header_table_.MaxInsertSizeWithoutEvictingGivenEntry( smallest_blocking_index)) { // If allowed, insert entry into dynamic table and refer to it. - sent_byte_count += - encoder_stream_sender_.SendInsertWithNameReference( - is_static, index, value); + encoder_stream_sender_.SendInsertWithNameReference(is_static, index, + value); auto entry = header_table_.InsertEntry(name, value); instructions.push_back(EncodeIndexedHeaderField( /* is_static = */ false, entry->InsertionIndex(), @@ -215,7 +213,7 @@ dynamic_table_insertion_blocked = true; } else { // If allowed, insert entry with name reference and refer to it. - sent_byte_count += encoder_stream_sender_.SendInsertWithNameReference( + encoder_stream_sender_.SendInsertWithNameReference( is_static, QpackAbsoluteIndexToEncoderStreamRelativeIndex( index, header_table_.inserted_entry_count()), @@ -258,9 +256,7 @@ smallest_blocking_index)) { dynamic_table_insertion_blocked = true; } else { - sent_byte_count += - encoder_stream_sender_.SendInsertWithoutNameReference(name, - value); + encoder_stream_sender_.SendInsertWithoutNameReference(name, value); auto entry = header_table_.InsertEntry(name, value); instructions.push_back(EncodeIndexedHeaderField( /* is_static = */ false, entry->InsertionIndex(), @@ -281,10 +277,7 @@ } } - encoder_stream_sender_.Flush(); - - // Use local |sent_byte_count| variable to avoid branching and dereferencing - // each time encoder stream data is sent. + const QuicByteCount sent_byte_count = encoder_stream_sender_.Flush(); if (encoder_stream_sent_byte_count) { *encoder_stream_sent_byte_count = sent_byte_count; }
diff --git a/quic/core/qpack/qpack_encoder_stream_sender.cc b/quic/core/qpack/qpack_encoder_stream_sender.cc index 8df0b03..4a7f12c 100644 --- a/quic/core/qpack/qpack_encoder_stream_sender.cc +++ b/quic/core/qpack/qpack_encoder_stream_sender.cc
@@ -15,7 +15,7 @@ QpackEncoderStreamSender::QpackEncoderStreamSender() : delegate_(nullptr) {} -QuicByteCount QpackEncoderStreamSender::SendInsertWithNameReference( +void QpackEncoderStreamSender::SendInsertWithNameReference( bool is_static, uint64_t name_index, QuicStringPiece value) { @@ -23,45 +23,42 @@ values_.varint = name_index; values_.value = value; - return Encode(InsertWithNameReferenceInstruction()); + instruction_encoder_.Encode(InsertWithNameReferenceInstruction(), values_, + &buffer_); } -QuicByteCount QpackEncoderStreamSender::SendInsertWithoutNameReference( +void QpackEncoderStreamSender::SendInsertWithoutNameReference( QuicStringPiece name, QuicStringPiece value) { values_.name = name; values_.value = value; - return Encode(InsertWithoutNameReferenceInstruction()); + instruction_encoder_.Encode(InsertWithoutNameReferenceInstruction(), values_, + &buffer_); } -QuicByteCount QpackEncoderStreamSender::SendDuplicate(uint64_t index) { +void QpackEncoderStreamSender::SendDuplicate(uint64_t index) { values_.varint = index; - return Encode(DuplicateInstruction()); + instruction_encoder_.Encode(DuplicateInstruction(), values_, &buffer_); } -QuicByteCount QpackEncoderStreamSender::SendSetDynamicTableCapacity( - uint64_t capacity) { +void QpackEncoderStreamSender::SendSetDynamicTableCapacity(uint64_t capacity) { values_.varint = capacity; - return Encode(SetDynamicTableCapacityInstruction()); + instruction_encoder_.Encode(SetDynamicTableCapacityInstruction(), values_, + &buffer_); } -void QpackEncoderStreamSender::Flush() { +QuicByteCount QpackEncoderStreamSender::Flush() { if (buffer_.empty()) { - return; + return 0; } delegate_->WriteStreamData(buffer_); + const QuicByteCount bytes_written = buffer_.size(); 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; + 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 5c4af7d..efbfbc6 100644 --- a/quic/core/qpack/qpack_encoder_stream_sender.h +++ b/quic/core/qpack/qpack_encoder_stream_sender.h
@@ -27,19 +27,20 @@ // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#rfc.section.5.2 // 5.2.1. Insert With Name Reference - QuicByteCount SendInsertWithNameReference(bool is_static, - uint64_t name_index, - QuicStringPiece value); + void SendInsertWithNameReference(bool is_static, + uint64_t name_index, + QuicStringPiece value); // 5.2.2. Insert Without Name Reference - QuicByteCount SendInsertWithoutNameReference(QuicStringPiece name, - QuicStringPiece value); + void SendInsertWithoutNameReference(QuicStringPiece name, + QuicStringPiece value); // 5.2.3. Duplicate - QuicByteCount SendDuplicate(uint64_t index); + void SendDuplicate(uint64_t index); // 5.2.4. Set Dynamic Table Capacity - QuicByteCount SendSetDynamicTableCapacity(uint64_t capacity); + void SendSetDynamicTableCapacity(uint64_t capacity); // Writes all buffered instructions on the encoder stream. - void Flush(); + // Returns the number of bytes written. + QuicByteCount Flush(); // delegate must be set if dynamic table capacity is not zero. void set_qpack_stream_sender_delegate(QpackStreamSenderDelegate* delegate) { @@ -47,10 +48,6 @@ } 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_;
diff --git a/quic/core/qpack/qpack_encoder_stream_sender_test.cc b/quic/core/qpack/qpack_encoder_stream_sender_test.cc index b1ddd97..80a6ed3 100644 --- a/quic/core/qpack/qpack_encoder_stream_sender_test.cc +++ b/quic/core/qpack/qpack_encoder_stream_sender_test.cc
@@ -30,23 +30,20 @@ // Static, index fits in prefix, empty value. std::string expected_encoded_data = QuicTextUtils::HexDecode("c500"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); - EXPECT_EQ(expected_encoded_data.size(), - stream_.SendInsertWithNameReference(true, 5, "")); - stream_.Flush(); + stream_.SendInsertWithNameReference(true, 5, ""); + EXPECT_EQ(expected_encoded_data.size(), 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(); + stream_.SendInsertWithNameReference(true, 2, "foo"); + EXPECT_EQ(expected_encoded_data.size(), 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(); + stream_.SendInsertWithNameReference(false, 137, "bar"); + EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); // Value length does not fit in prefix. // 'Z' would be Huffman encoded to 8 bits, so no Huffman encoding is used. @@ -56,33 +53,28 @@ "5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a" "5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); - EXPECT_EQ( - expected_encoded_data.size(), - stream_.SendInsertWithNameReference(false, 42, std::string(127, 'Z'))); - stream_.Flush(); + stream_.SendInsertWithNameReference(false, 42, std::string(127, 'Z')); + EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); } TEST_F(QpackEncoderStreamSenderTest, InsertWithoutNameReference) { // Empty name and value. std::string expected_encoded_data = QuicTextUtils::HexDecode("4000"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); - EXPECT_EQ(expected_encoded_data.size(), - stream_.SendInsertWithoutNameReference("", "")); - stream_.Flush(); + stream_.SendInsertWithoutNameReference("", ""); + EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); // 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(); + stream_.SendInsertWithoutNameReference("foo", "foo"); + EXPECT_EQ(expected_encoded_data.size(), 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(); + stream_.SendInsertWithoutNameReference("bar", "bar"); + EXPECT_EQ(expected_encoded_data.size(), 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. @@ -93,74 +85,67 @@ "5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a" "5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); - EXPECT_EQ(expected_encoded_data.size(), - stream_.SendInsertWithoutNameReference(std::string(31, 'Z'), - std::string(127, 'Z'))); - stream_.Flush(); + stream_.SendInsertWithoutNameReference(std::string(31, 'Z'), + std::string(127, 'Z')); + EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); } TEST_F(QpackEncoderStreamSenderTest, Duplicate) { // Small index fits in prefix. 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(); + stream_.SendDuplicate(17); + EXPECT_EQ(expected_encoded_data.size(), 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(); + stream_.SendDuplicate(500); + EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); } TEST_F(QpackEncoderStreamSenderTest, SetDynamicTableCapacity) { // Small capacity fits in prefix. std::string expected_encoded_data = QuicTextUtils::HexDecode("31"); EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); - EXPECT_EQ(expected_encoded_data.size(), - stream_.SendSetDynamicTableCapacity(17)); - stream_.Flush(); + stream_.SendSetDynamicTableCapacity(17); + EXPECT_EQ(expected_encoded_data.size(), 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(); + stream_.SendSetDynamicTableCapacity(500); + EXPECT_EQ(expected_encoded_data.size(), 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, "")); + 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")); + 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")); + stream_.SendInsertWithoutNameReference("foo", "foo"); // Duplicate entry. - std::string expected_encoded_data4 = QuicTextUtils::HexDecode("11"); - EXPECT_EQ(expected_encoded_data4.size(), stream_.SendDuplicate(17)); + stream_.SendDuplicate(17); - EXPECT_CALL(delegate_, - WriteStreamData(Eq(QuicStrCat( - expected_encoded_data1 + expected_encoded_data2 + - expected_encoded_data3 + expected_encoded_data4)))); - stream_.Flush(); + std::string expected_encoded_data = QuicTextUtils::HexDecode( + "c500" // Insert entry with static name reference. + "c28294e7" // Insert entry with static name reference. + "6294e78294e7" // Insert literal entry. + "11"); // Duplicate entry. + + EXPECT_CALL(delegate_, WriteStreamData(Eq(expected_encoded_data))); + EXPECT_EQ(expected_encoded_data.size(), stream_.Flush()); } // No writes should happen if QpackEncoderStreamSender::Flush() is called // when the buffer is empty. TEST_F(QpackEncoderStreamSenderTest, FlushEmpty) { - stream_.Flush(); + EXPECT_EQ(0u, stream_.Flush()); } } // namespace