Make QpackEncoder interface not progressive.
When the QPACK implementation was originally written, an
HpackEncoder::ProgressiveEncoder interface was used for encoding. However, the
way it is used in QuicSpdyStream, it is clear now that encoding can be done in a
single pass into a std::string object. QuicStream::WriteOrBufferData() does not
lend itself easily to notifying the QPACK encoder that it is ready to take more
data. Instead it is more natural to encode the entire header block and buffer
that in QuicStreamSendBuffer. Also note that this should not cost more memory
than doing progressive encoding, because the SpdyHeaderBlock object would need
to be kept around if progressive encoding was used.
This CL introduces no functional change, it just changes the interface, and
moves the code that calls Next() from QuicSpdyStream into QpackEncoder. A
follow-up CL will clean up QpackEncoder internals.
gfe-relnote: n/a, QUIC v99-only change.
PiperOrigin-RevId: 254323435
Change-Id: Ic3d203ed9f019ad01dba61d407a6e73bde5d12d1
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index 0aa796a..93eceb0 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -2293,14 +2293,8 @@
NoopQpackStreamSenderDelegate encoder_stream_sender_delegate;
QpackEncoder qpack_encoder(&decoder_stream_error_delegate,
&encoder_stream_sender_delegate);
- auto progressive_encoder =
+ std::string encoded_headers =
qpack_encoder.EncodeHeaderList(/* stream_id = */ 0, &headers);
- std::string encoded_headers;
- while (progressive_encoder->HasNext()) {
- progressive_encoder->Next(
- /* max_encoded_bytes = */ std::numeric_limits<size_t>::max(),
- &encoded_headers);
- }
header_size = encoded_headers.size();
}
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index 02601ba..92138a4 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -863,14 +863,8 @@
}
// Encode header list.
- auto progressive_encoder = spdy_session_->qpack_encoder()->EncodeHeaderList(
- /* stream_id = */ id(), &header_block);
- std::string encoded_headers;
- while (progressive_encoder->HasNext()) {
- progressive_encoder->Next(
- /* max_encoded_bytes = */ std::numeric_limits<size_t>::max(),
- &encoded_headers);
- }
+ std::string encoded_headers =
+ spdy_session_->qpack_encoder()->EncodeHeaderList(id(), &header_block);
// Write HEADERS frame.
std::unique_ptr<char[]> headers_frame_header;
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index de13108..c627487 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -162,13 +162,7 @@
NoopQpackStreamSenderDelegate encoder_stream_sender_delegate;
auto qpack_encoder = QuicMakeUnique<QpackEncoder>(
session_.get(), &encoder_stream_sender_delegate);
- auto progressive_encoder = qpack_encoder->EncodeHeaderList(id, header);
- std::string encoded_headers;
- while (progressive_encoder->HasNext()) {
- progressive_encoder->Next(std::numeric_limits<size_t>::max(),
- &encoded_headers);
- }
- return encoded_headers;
+ return qpack_encoder->EncodeHeaderList(id, header);
}
void Initialize(bool stream_should_process_data) {
diff --git a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
index 0d74c2e..a677ab4 100644
--- a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
+++ b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
@@ -128,9 +128,10 @@
// Encode header list.
NoopDecoderStreamErrorDelegate decoder_stream_error_delegate;
NoopQpackStreamSenderDelegate encoder_stream_sender_delegate;
- std::string encoded_header_block = QpackEncode(
- &decoder_stream_error_delegate, &encoder_stream_sender_delegate,
- fragment_size_generator, &header_list);
+ QpackEncoder encoder(&decoder_stream_error_delegate,
+ &encoder_stream_sender_delegate);
+ std::string encoded_header_block =
+ encoder.EncodeHeaderList(/* stream_id = */ 1, &header_list);
// Decode header block.
TestHeadersHandler handler;
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc
index 92bf0dc..67e409e 100644
--- a/quic/core/qpack/qpack_encoder.cc
+++ b/quic/core/qpack/qpack_encoder.cc
@@ -24,11 +24,18 @@
QpackEncoder::~QpackEncoder() {}
-std::unique_ptr<spdy::HpackEncoder::ProgressiveEncoder>
-QpackEncoder::EncodeHeaderList(QuicStreamId stream_id,
- const spdy::SpdyHeaderBlock* header_list) {
- return QuicMakeUnique<QpackProgressiveEncoder>(
+std::string QpackEncoder::EncodeHeaderList(
+ QuicStreamId stream_id,
+ const spdy::SpdyHeaderBlock* header_list) {
+ std::string encoded_headers;
+ auto progressive_encoder = QuicMakeUnique<QpackProgressiveEncoder>(
stream_id, &header_table_, &encoder_stream_sender_, header_list);
+ while (progressive_encoder->HasNext()) {
+ progressive_encoder->Next(
+ /* max_encoded_bytes = */ std::numeric_limits<size_t>::max(),
+ &encoded_headers);
+ }
+ return encoded_headers;
}
void QpackEncoder::DecodeDecoderStreamData(QuicStringPiece data) {
diff --git a/quic/core/qpack/qpack_encoder.h b/quic/core/qpack/qpack_encoder.h
index a14d91b..7ed5808 100644
--- a/quic/core/qpack/qpack_encoder.h
+++ b/quic/core/qpack/qpack_encoder.h
@@ -7,6 +7,7 @@
#include <cstdint>
#include <memory>
+#include <string>
#include "net/third_party/quiche/src/quic/core/qpack/qpack_decoder_stream_receiver.h"
#include "net/third_party/quiche/src/quic/core/qpack/qpack_encoder_stream_sender.h"
@@ -14,7 +15,6 @@
#include "net/third_party/quiche/src/quic/core/quic_types.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_string_piece.h"
-#include "net/third_party/quiche/src/spdy/core/hpack/hpack_encoder.h"
namespace spdy {
@@ -25,8 +25,6 @@
namespace quic {
// QPACK encoder class. Exactly one instance should exist per QUIC connection.
-// This class vends a new QpackProgressiveEncoder instance for each new header
-// list to be encoded.
class QUIC_EXPORT_PRIVATE QpackEncoder
: public QpackDecoderStreamReceiver::Delegate {
public:
@@ -44,12 +42,9 @@
QpackStreamSenderDelegate* encoder_stream_sender_delegate);
~QpackEncoder() override;
- // This factory method is called to start encoding a header list.
- // |*header_list| must remain valid and must not change
- // during the lifetime of the returned ProgressiveEncoder instance.
- std::unique_ptr<spdy::HpackEncoder::ProgressiveEncoder> EncodeHeaderList(
- QuicStreamId stream_id,
- const spdy::SpdyHeaderBlock* header_list);
+ // Encode a header list.
+ std::string EncodeHeaderList(QuicStreamId stream_id,
+ const spdy::SpdyHeaderBlock* header_list);
// Decode data received on the decoder stream.
void DecodeDecoderStreamData(QuicStringPiece data);
diff --git a/quic/core/qpack/qpack_encoder_test.cc b/quic/core/qpack/qpack_encoder_test.cc
index add6a74..8633529 100644
--- a/quic/core/qpack/qpack_encoder_test.cc
+++ b/quic/core/qpack/qpack_encoder_test.cc
@@ -20,37 +20,29 @@
namespace test {
namespace {
-class QpackEncoderTest : public QuicTestWithParam<FragmentMode> {
+class QpackEncoderTest : public QuicTest {
protected:
- QpackEncoderTest() : fragment_mode_(GetParam()) {}
+ QpackEncoderTest() = default;
~QpackEncoderTest() override = default;
std::string Encode(const spdy::SpdyHeaderBlock* header_list) {
- return QpackEncode(
- &decoder_stream_error_delegate_, &encoder_stream_sender_delegate_,
- FragmentModeToFragmentSizeGenerator(fragment_mode_), header_list);
+ QpackEncoder encoder(&decoder_stream_error_delegate_,
+ &encoder_stream_sender_delegate_);
+ return encoder.EncodeHeaderList(/* stream_id = */ 1, header_list);
}
StrictMock<MockDecoderStreamErrorDelegate> decoder_stream_error_delegate_;
NoopQpackStreamSenderDelegate encoder_stream_sender_delegate_;
-
- private:
- const FragmentMode fragment_mode_;
};
-INSTANTIATE_TEST_SUITE_P(,
- QpackEncoderTest,
- Values(FragmentMode::kSingleChunk,
- FragmentMode::kOctetByOctet));
-
-TEST_P(QpackEncoderTest, Empty) {
+TEST_F(QpackEncoderTest, Empty) {
spdy::SpdyHeaderBlock header_list;
std::string output = Encode(&header_list);
EXPECT_EQ(QuicTextUtils::HexDecode("0000"), output);
}
-TEST_P(QpackEncoderTest, EmptyName) {
+TEST_F(QpackEncoderTest, EmptyName) {
spdy::SpdyHeaderBlock header_list;
header_list[""] = "foo";
std::string output = Encode(&header_list);
@@ -58,7 +50,7 @@
EXPECT_EQ(QuicTextUtils::HexDecode("0000208294e7"), output);
}
-TEST_P(QpackEncoderTest, EmptyValue) {
+TEST_F(QpackEncoderTest, EmptyValue) {
spdy::SpdyHeaderBlock header_list;
header_list["foo"] = "";
std::string output = Encode(&header_list);
@@ -66,7 +58,7 @@
EXPECT_EQ(QuicTextUtils::HexDecode("00002a94e700"), output);
}
-TEST_P(QpackEncoderTest, EmptyNameAndValue) {
+TEST_F(QpackEncoderTest, EmptyNameAndValue) {
spdy::SpdyHeaderBlock header_list;
header_list[""] = "";
std::string output = Encode(&header_list);
@@ -74,7 +66,7 @@
EXPECT_EQ(QuicTextUtils::HexDecode("00002000"), output);
}
-TEST_P(QpackEncoderTest, Simple) {
+TEST_F(QpackEncoderTest, Simple) {
spdy::SpdyHeaderBlock header_list;
header_list["foo"] = "bar";
std::string output = Encode(&header_list);
@@ -82,7 +74,7 @@
EXPECT_EQ(QuicTextUtils::HexDecode("00002a94e703626172"), output);
}
-TEST_P(QpackEncoderTest, Multiple) {
+TEST_F(QpackEncoderTest, Multiple) {
spdy::SpdyHeaderBlock header_list;
header_list["foo"] = "bar";
// 'Z' would be Huffman encoded to 8 bits, so no Huffman encoding is used.
@@ -104,7 +96,7 @@
output);
}
-TEST_P(QpackEncoderTest, StaticTable) {
+TEST_F(QpackEncoderTest, StaticTable) {
{
spdy::SpdyHeaderBlock header_list;
header_list[":method"] = "GET";
@@ -134,26 +126,7 @@
}
}
-TEST_P(QpackEncoderTest, SimpleIndexed) {
- spdy::SpdyHeaderBlock header_list;
- header_list[":path"] = "/";
-
- QpackEncoder encoder(&decoder_stream_error_delegate_,
- &encoder_stream_sender_delegate_);
- auto progressive_encoder =
- encoder.EncodeHeaderList(/* stream_id = */ 1, &header_list);
- EXPECT_TRUE(progressive_encoder->HasNext());
-
- // This indexed header field takes exactly three bytes:
- // two for the prefix, one for the indexed static entry.
- std::string output;
- progressive_encoder->Next(3, &output);
-
- EXPECT_EQ(QuicTextUtils::HexDecode("0000c1"), output);
- EXPECT_FALSE(progressive_encoder->HasNext());
-}
-
-TEST_P(QpackEncoderTest, DecoderStreamError) {
+TEST_F(QpackEncoderTest, DecoderStreamError) {
EXPECT_CALL(decoder_stream_error_delegate_,
OnDecoderStreamError(Eq("Encoded integer too large.")));
@@ -163,7 +136,7 @@
QuicTextUtils::HexDecode("ffffffffffffffffffffff"));
}
-TEST_P(QpackEncoderTest, SplitAlongNullCharacter) {
+TEST_F(QpackEncoderTest, SplitAlongNullCharacter) {
spdy::SpdyHeaderBlock header_list;
header_list["foo"] = QuicStringPiece("bar\0bar\0baz", 11);
std::string output = Encode(&header_list);
diff --git a/quic/core/qpack/qpack_encoder_test_utils.cc b/quic/core/qpack/qpack_encoder_test_utils.cc
index a5fdbea..d91d3d1 100644
--- a/quic/core/qpack/qpack_encoder_test_utils.cc
+++ b/quic/core/qpack/qpack_encoder_test_utils.cc
@@ -12,23 +12,5 @@
void NoopDecoderStreamErrorDelegate::OnDecoderStreamError(
QuicStringPiece /*error_message*/) {}
-std::string QpackEncode(
- QpackEncoder::DecoderStreamErrorDelegate* decoder_stream_error_delegate,
- QpackStreamSenderDelegate* encoder_stream_sender_delegate,
- const FragmentSizeGenerator& fragment_size_generator,
- const spdy::SpdyHeaderBlock* header_list) {
- QpackEncoder encoder(decoder_stream_error_delegate,
- encoder_stream_sender_delegate);
- auto progressive_encoder =
- encoder.EncodeHeaderList(/* stream_id = */ 1, header_list);
-
- std::string output;
- while (progressive_encoder->HasNext()) {
- progressive_encoder->Next(fragment_size_generator(), &output);
- }
-
- return output;
-}
-
} // namespace test
} // namespace quic
diff --git a/quic/core/qpack/qpack_encoder_test_utils.h b/quic/core/qpack/qpack_encoder_test_utils.h
index f897d45..b1103da 100644
--- a/quic/core/qpack/qpack_encoder_test_utils.h
+++ b/quic/core/qpack/qpack_encoder_test_utils.h
@@ -34,12 +34,6 @@
MOCK_METHOD1(OnDecoderStreamError, void(QuicStringPiece error_message));
};
-std::string QpackEncode(
- QpackEncoder::DecoderStreamErrorDelegate* decoder_stream_error_delegate,
- QpackStreamSenderDelegate* encoder_stream_sender_delegate,
- const FragmentSizeGenerator& fragment_size_generator,
- const spdy::SpdyHeaderBlock* header_list);
-
} // namespace test
} // namespace quic
diff --git a/quic/core/qpack/qpack_round_trip_test.cc b/quic/core/qpack/qpack_round_trip_test.cc
index 4fc1a38..1e9de2d 100644
--- a/quic/core/qpack/qpack_round_trip_test.cc
+++ b/quic/core/qpack/qpack_round_trip_test.cc
@@ -20,29 +20,25 @@
namespace test {
namespace {
-class QpackRoundTripTest
- : public QuicTestWithParam<std::tuple<FragmentMode, FragmentMode>> {
+class QpackRoundTripTest : public QuicTestWithParam<FragmentMode> {
public:
- QpackRoundTripTest()
- : encoding_fragment_mode_(std::get<0>(GetParam())),
- decoding_fragment_mode_(std::get<1>(GetParam())) {}
+ QpackRoundTripTest() = default;
~QpackRoundTripTest() override = default;
spdy::SpdyHeaderBlock EncodeThenDecode(
const spdy::SpdyHeaderBlock& header_list) {
NoopDecoderStreamErrorDelegate decoder_stream_error_delegate;
NoopQpackStreamSenderDelegate encoder_stream_sender_delegate;
- std::string encoded_header_block = QpackEncode(
- &decoder_stream_error_delegate, &encoder_stream_sender_delegate,
- FragmentModeToFragmentSizeGenerator(encoding_fragment_mode_),
- &header_list);
+ QpackEncoder encoder(&decoder_stream_error_delegate,
+ &encoder_stream_sender_delegate);
+ std::string encoded_header_block =
+ encoder.EncodeHeaderList(/* stream_id = */ 1, &header_list);
TestHeadersHandler handler;
NoopEncoderStreamErrorDelegate encoder_stream_error_delegate;
NoopQpackStreamSenderDelegate decoder_stream_sender_delegate;
QpackDecode(&encoder_stream_error_delegate, &decoder_stream_sender_delegate,
- &handler,
- FragmentModeToFragmentSizeGenerator(decoding_fragment_mode_),
+ &handler, FragmentModeToFragmentSizeGenerator(GetParam()),
encoded_header_block);
EXPECT_TRUE(handler.decoding_completed());
@@ -50,17 +46,12 @@
return handler.ReleaseHeaderList();
}
-
- private:
- const FragmentMode encoding_fragment_mode_;
- const FragmentMode decoding_fragment_mode_;
};
-INSTANTIATE_TEST_SUITE_P(
- ,
- QpackRoundTripTest,
- Combine(Values(FragmentMode::kSingleChunk, FragmentMode::kOctetByOctet),
- Values(FragmentMode::kSingleChunk, FragmentMode::kOctetByOctet)));
+INSTANTIATE_TEST_SUITE_P(,
+ QpackRoundTripTest,
+ Values(FragmentMode::kSingleChunk,
+ FragmentMode::kOctetByOctet));
TEST_P(QpackRoundTripTest, Empty) {
spdy::SpdyHeaderBlock header_list;
