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
