Modify QpackInstructionEncoder API. QpackInstructionEncoder does not need to support progressive encoding. This CL modifies the API, a follow-up CL will clean up internals. Taking |&output| and appending is more performant than returning std::string when encoding headers, which involve concatenating a large number of instructions. gfe-relnote: n/a, QUIC v99-only change. PiperOrigin-RevId: 254497735 Change-Id: I62d74a0523220e5730ffc85ea66ab071345fc8fb
diff --git a/quic/core/qpack/qpack_decoder_stream_sender.cc b/quic/core/qpack/qpack_decoder_stream_sender.cc index a38c6d5..4586fd3 100644 --- a/quic/core/qpack/qpack_decoder_stream_sender.cc +++ b/quic/core/qpack/qpack_decoder_stream_sender.cc
@@ -22,13 +22,8 @@ void QpackDecoderStreamSender::SendInsertCountIncrement(uint64_t increment) { instruction_encoder_.set_varint(increment); - instruction_encoder_.Encode(InsertCountIncrementInstruction()); - std::string output; - - instruction_encoder_.Next(std::numeric_limits<size_t>::max(), &output); - DCHECK(!instruction_encoder_.HasNext()); - + instruction_encoder_.Encode(InsertCountIncrementInstruction(), &output); delegate_->WriteStreamData(output); } @@ -36,26 +31,16 @@ QuicStreamId stream_id) { instruction_encoder_.set_varint(stream_id); - instruction_encoder_.Encode(HeaderAcknowledgementInstruction()); - std::string output; - - instruction_encoder_.Next(std::numeric_limits<size_t>::max(), &output); - DCHECK(!instruction_encoder_.HasNext()); - + instruction_encoder_.Encode(HeaderAcknowledgementInstruction(), &output); delegate_->WriteStreamData(output); } void QpackDecoderStreamSender::SendStreamCancellation(QuicStreamId stream_id) { instruction_encoder_.set_varint(stream_id); - instruction_encoder_.Encode(StreamCancellationInstruction()); - std::string output; - - instruction_encoder_.Next(std::numeric_limits<size_t>::max(), &output); - DCHECK(!instruction_encoder_.HasNext()); - + instruction_encoder_.Encode(StreamCancellationInstruction(), &output); delegate_->WriteStreamData(output); }
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc index d5bea0d..c5997b1 100644 --- a/quic/core/qpack/qpack_encoder.cc +++ b/quic/core/qpack/qpack_encoder.cc
@@ -38,11 +38,7 @@ instruction_encoder.set_varint2(0); instruction_encoder.set_s_bit(false); - instruction_encoder.Encode(QpackPrefixInstruction()); - DCHECK(instruction_encoder.HasNext()); - instruction_encoder.Next(std::numeric_limits<size_t>::max(), - &encoded_headers); - DCHECK(!instruction_encoder.HasNext()); + instruction_encoder.Encode(QpackPrefixInstruction(), &encoded_headers); for (const auto& header : ValueSplittingHeaderList(header_list)) { QuicStringPiece name = header.first; @@ -61,7 +57,8 @@ instruction_encoder.set_s_bit(is_static); instruction_encoder.set_varint(index); - instruction_encoder.Encode(QpackIndexedHeaderFieldInstruction()); + instruction_encoder.Encode(QpackIndexedHeaderFieldInstruction(), + &encoded_headers); break; case QpackHeaderTable::MatchType::kName: @@ -72,22 +69,19 @@ instruction_encoder.set_value(value); instruction_encoder.Encode( - QpackLiteralHeaderFieldNameReferenceInstruction()); + QpackLiteralHeaderFieldNameReferenceInstruction(), + &encoded_headers); break; case QpackHeaderTable::MatchType::kNoMatch: instruction_encoder.set_name(name); instruction_encoder.set_value(value); - instruction_encoder.Encode(QpackLiteralHeaderFieldInstruction()); + instruction_encoder.Encode(QpackLiteralHeaderFieldInstruction(), + &encoded_headers); break; } - - DCHECK(instruction_encoder.HasNext()); - instruction_encoder.Next(std::numeric_limits<size_t>::max(), - &encoded_headers); - DCHECK(!instruction_encoder.HasNext()); } return encoded_headers;
diff --git a/quic/core/qpack/qpack_encoder_stream_sender.cc b/quic/core/qpack/qpack_encoder_stream_sender.cc index e4299ed..ca1da4f 100644 --- a/quic/core/qpack/qpack_encoder_stream_sender.cc +++ b/quic/core/qpack/qpack_encoder_stream_sender.cc
@@ -27,13 +27,8 @@ instruction_encoder_.set_varint(name_index); instruction_encoder_.set_value(value); - instruction_encoder_.Encode(InsertWithNameReferenceInstruction()); - std::string output; - - instruction_encoder_.Next(std::numeric_limits<size_t>::max(), &output); - DCHECK(!instruction_encoder_.HasNext()); - + instruction_encoder_.Encode(InsertWithNameReferenceInstruction(), &output); delegate_->WriteStreamData(output); } @@ -43,39 +38,24 @@ instruction_encoder_.set_name(name); instruction_encoder_.set_value(value); - instruction_encoder_.Encode(InsertWithoutNameReferenceInstruction()); - std::string output; - - instruction_encoder_.Next(std::numeric_limits<size_t>::max(), &output); - DCHECK(!instruction_encoder_.HasNext()); - + instruction_encoder_.Encode(InsertWithoutNameReferenceInstruction(), &output); delegate_->WriteStreamData(output); } void QpackEncoderStreamSender::SendDuplicate(uint64_t index) { instruction_encoder_.set_varint(index); - instruction_encoder_.Encode(DuplicateInstruction()); - std::string output; - - instruction_encoder_.Next(std::numeric_limits<size_t>::max(), &output); - DCHECK(!instruction_encoder_.HasNext()); - + instruction_encoder_.Encode(DuplicateInstruction(), &output); delegate_->WriteStreamData(output); } void QpackEncoderStreamSender::SendSetDynamicTableCapacity(uint64_t capacity) { instruction_encoder_.set_varint(capacity); - instruction_encoder_.Encode(SetDynamicTableCapacityInstruction()); - std::string output; - - instruction_encoder_.Next(std::numeric_limits<size_t>::max(), &output); - DCHECK(!instruction_encoder_.HasNext()); - + instruction_encoder_.Encode(SetDynamicTableCapacityInstruction(), &output); delegate_->WriteStreamData(output); }
diff --git a/quic/core/qpack/qpack_instruction_encoder.cc b/quic/core/qpack/qpack_instruction_encoder.cc index 0da9567..1e00d96 100644 --- a/quic/core/qpack/qpack_instruction_encoder.cc +++ b/quic/core/qpack/qpack_instruction_encoder.cc
@@ -4,6 +4,8 @@ #include "net/third_party/quiche/src/quic/core/qpack/qpack_instruction_encoder.h" +#include <limits> + #include "net/third_party/quiche/src/http2/hpack/huffman/hpack_huffman_encoder.h" #include "net/third_party/quiche/src/quic/platform/api/quic_logging.h" #include "net/third_party/quiche/src/quic/platform/api/quic_string_utils.h" @@ -18,7 +20,16 @@ state_(State::kOpcode), instruction_(nullptr) {} -void QpackInstructionEncoder::Encode(const QpackInstruction* instruction) { +void QpackInstructionEncoder::Encode(const QpackInstruction* instruction, + std::string* output) { + StartEncode(instruction); + DCHECK(HasNext()); + + Next(std::numeric_limits<size_t>::max(), output); + DCHECK(!HasNext()); +} + +void QpackInstructionEncoder::StartEncode(const QpackInstruction* instruction) { DCHECK(!HasNext()); state_ = State::kOpcode;
diff --git a/quic/core/qpack/qpack_instruction_encoder.h b/quic/core/qpack/qpack_instruction_encoder.h index 917378e..1da850f 100644 --- a/quic/core/qpack/qpack_instruction_encoder.h +++ b/quic/core/qpack/qpack_instruction_encoder.h
@@ -33,9 +33,13 @@ void set_name(QuicStringPiece name) { name_ = name; } void set_value(QuicStringPiece value) { value_ = value; } + // Append encoded instruction to |output|. + void Encode(const QpackInstruction* instruction, std::string* output); + + private: // Start encoding an instruction. Must only be called after the previous // instruction has been completely encoded. - void Encode(const QpackInstruction* instruction); + void StartEncode(const QpackInstruction* instruction); // Returns true iff more data remains to be encoded for the current // instruction. Returns false if there is no current instruction, that is, if @@ -47,7 +51,6 @@ // returns true. |max_encoded_bytes| must be positive. void Next(size_t max_encoded_bytes, std::string* output); - private: enum class State { // Write instruction opcode to |byte_|. kOpcode,
diff --git a/quic/core/qpack/qpack_instruction_encoder_test.cc b/quic/core/qpack/qpack_instruction_encoder_test.cc index 738a1b7..d921fa6 100644 --- a/quic/core/qpack/qpack_instruction_encoder_test.cc +++ b/quic/core/qpack/qpack_instruction_encoder_test.cc
@@ -4,7 +4,6 @@ #include "net/third_party/quiche/src/quic/core/qpack/qpack_instruction_encoder.h" -#include "net/third_party/quiche/src/quic/core/qpack/qpack_test_utils.h" #include "net/third_party/quiche/src/quic/platform/api/quic_logging.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" #include "net/third_party/quiche/src/quic/platform/api/quic_text_utils.h" @@ -15,49 +14,46 @@ namespace test { namespace { -class QpackInstructionEncoderTest : public QuicTestWithParam<FragmentMode> { +class QpackInstructionEncoderTest : public QuicTest { protected: - QpackInstructionEncoderTest() : fragment_mode_(GetParam()) {} + QpackInstructionEncoderTest() : verified_position_(0) {} ~QpackInstructionEncoderTest() override = default; - // Encode |instruction| with fragment sizes dictated by |fragment_mode_|. - std::string EncodeInstruction(const QpackInstruction* instruction) { - EXPECT_FALSE(encoder_.HasNext()); + // Append encoded |instruction| to |output_|. + void EncodeInstruction(const QpackInstruction* instruction) { + encoder_.Encode(instruction, &output_); + } - FragmentSizeGenerator fragment_size_generator = - FragmentModeToFragmentSizeGenerator(fragment_mode_); - std::string output; - encoder_.Encode(instruction); - while (encoder_.HasNext()) { - encoder_.Next(fragment_size_generator(), &output); - } - - return output; + // Compare substring appended to |output_| since last EncodedSegmentMatches() + // call against hex-encoded argument. + bool EncodedSegmentMatches(QuicStringPiece hex_encoded_expected_substring) { + auto recently_encoded = QuicStringPiece(output_).substr(verified_position_); + auto expected = QuicTextUtils::HexDecode(hex_encoded_expected_substring); + verified_position_ = output_.size(); + return recently_encoded == expected; } QpackInstructionEncoder encoder_; private: - const FragmentMode fragment_mode_; + std::string output_; + std::string::size_type verified_position_; }; -INSTANTIATE_TEST_SUITE_P(, - QpackInstructionEncoderTest, - Values(FragmentMode::kSingleChunk, - FragmentMode::kOctetByOctet)); - -TEST_P(QpackInstructionEncoderTest, Varint) { +TEST_F(QpackInstructionEncoderTest, Varint) { const QpackInstruction instruction{QpackInstructionOpcode{0x00, 0x80}, {{QpackInstructionFieldType::kVarint, 7}}}; encoder_.set_varint(5); - EXPECT_EQ(QuicTextUtils::HexDecode("05"), EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("05")); encoder_.set_varint(127); - EXPECT_EQ(QuicTextUtils::HexDecode("7f00"), EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("7f00")); } -TEST_P(QpackInstructionEncoderTest, SBitAndTwoVarint2) { +TEST_F(QpackInstructionEncoderTest, SBitAndTwoVarint2) { const QpackInstruction instruction{ QpackInstructionOpcode{0x80, 0xc0}, {{QpackInstructionFieldType::kSbit, 0x20}, @@ -67,16 +63,17 @@ encoder_.set_s_bit(true); encoder_.set_varint(5); encoder_.set_varint2(200); - EXPECT_EQ(QuicTextUtils::HexDecode("a5c8"), EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("a5c8")); encoder_.set_s_bit(false); encoder_.set_varint(31); encoder_.set_varint2(356); - EXPECT_EQ(QuicTextUtils::HexDecode("9f00ff65"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("9f00ff65")); } -TEST_P(QpackInstructionEncoderTest, SBitAndVarintAndValue) { +TEST_F(QpackInstructionEncoderTest, SBitAndVarintAndValue) { const QpackInstruction instruction{QpackInstructionOpcode{0xc0, 0xc0}, {{QpackInstructionFieldType::kSbit, 0x20}, {QpackInstructionFieldType::kVarint, 5}, @@ -85,49 +82,51 @@ encoder_.set_s_bit(true); encoder_.set_varint(100); encoder_.set_value("foo"); - EXPECT_EQ(QuicTextUtils::HexDecode("ff458294e7"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("ff458294e7")); encoder_.set_s_bit(false); encoder_.set_varint(3); encoder_.set_value("bar"); - EXPECT_EQ(QuicTextUtils::HexDecode("c303626172"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("c303626172")); } -TEST_P(QpackInstructionEncoderTest, Name) { +TEST_F(QpackInstructionEncoderTest, Name) { const QpackInstruction instruction{QpackInstructionOpcode{0xe0, 0xe0}, {{QpackInstructionFieldType::kName, 4}}}; encoder_.set_name(""); - EXPECT_EQ(QuicTextUtils::HexDecode("e0"), EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("e0")); encoder_.set_name("foo"); - EXPECT_EQ(QuicTextUtils::HexDecode("f294e7"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("f294e7")); encoder_.set_name("bar"); - EXPECT_EQ(QuicTextUtils::HexDecode("e3626172"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("e3626172")); } -TEST_P(QpackInstructionEncoderTest, Value) { +TEST_F(QpackInstructionEncoderTest, Value) { const QpackInstruction instruction{QpackInstructionOpcode{0xf0, 0xf0}, {{QpackInstructionFieldType::kValue, 3}}}; encoder_.set_value(""); - EXPECT_EQ(QuicTextUtils::HexDecode("f0"), EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("f0")); encoder_.set_value("foo"); - EXPECT_EQ(QuicTextUtils::HexDecode("fa94e7"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("fa94e7")); encoder_.set_value("bar"); - EXPECT_EQ(QuicTextUtils::HexDecode("f3626172"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("f3626172")); } -TEST_P(QpackInstructionEncoderTest, SBitAndNameAndValue) { +TEST_F(QpackInstructionEncoderTest, SBitAndNameAndValue) { const QpackInstruction instruction{QpackInstructionOpcode{0xf0, 0xf0}, {{QpackInstructionFieldType::kSbit, 0x08}, {QpackInstructionFieldType::kName, 2}, @@ -136,13 +135,14 @@ encoder_.set_s_bit(false); encoder_.set_name(""); encoder_.set_value(""); - EXPECT_EQ(QuicTextUtils::HexDecode("f000"), EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("f000")); encoder_.set_s_bit(true); encoder_.set_name("foo"); encoder_.set_value("bar"); - EXPECT_EQ(QuicTextUtils::HexDecode("fe94e703626172"), - EncodeInstruction(&instruction)); + EncodeInstruction(&instruction); + EXPECT_TRUE(EncodedSegmentMatches("fe94e703626172")); } } // namespace