Improves metadata serialization by allocating less memory up front. To accomplish this, we add a `ChunkedBuffer` API to allow the caller to request an append region in which to write data. PiperOrigin-RevId: 845972726
diff --git a/quiche/http2/adapter/chunked_buffer.cc b/quiche/http2/adapter/chunked_buffer.cc index 5ccb882..40f40a9 100644 --- a/quiche/http2/adapter/chunked_buffer.cc +++ b/quiche/http2/adapter/chunked_buffer.cc
@@ -21,6 +21,7 @@ } // namespace void ChunkedBuffer::Append(absl::string_view data) { + total_size_ += data.size(); // Appends the data by copying it. const size_t to_copy = std::min(TailBytesFree(), data.size()); if (to_copy > 0) { @@ -32,6 +33,7 @@ } void ChunkedBuffer::Append(std::unique_ptr<char[]> data, size_t size) { + total_size_ += size; if (TailBytesFree() >= size) { // Copies the data into the existing last chunk, since it will fit. Chunk& c = chunks_.back(); @@ -47,6 +49,21 @@ chunks_.push_back({std::move(data), size, v}); } +ChunkedBuffer::AppendRegion::~AppendRegion() { + Chunk& c = parent->chunks_.back(); + c.live = absl::string_view(c.live.data(), c.live.size() + written); + parent->total_size_ += written; +} + +ChunkedBuffer::AppendRegion ChunkedBuffer::GetAppendRegion() { + if (TailBytesFree() == 0) { + EnsureTailBytesFree(kDefaultChunkSize); + } + Chunk& c = chunks_.back(); + return AppendRegion(c.data.get() + c.LiveDataOffset() + c.live.size(), + c.TailBytesFree(), 0, this); +} + absl::string_view ChunkedBuffer::GetPrefix() const { if (chunks_.empty()) { return ""; @@ -69,6 +86,7 @@ const size_t to_remove = std::min(n, c.live.size()); c.RemovePrefix(to_remove); n -= to_remove; + total_size_ -= to_remove; if (c.Empty()) { TrimFirstChunk(); } @@ -112,7 +130,8 @@ } const size_t to_allocate = RoundUpToNearestKilobyte(n); auto data = std::unique_ptr<char[]>(new char[to_allocate]); - chunks_.push_back({std::move(data), to_allocate, ""}); + absl::string_view live(data.get(), 0); + chunks_.push_back({std::move(data), to_allocate, live}); } void ChunkedBuffer::TrimFirstChunk() {
diff --git a/quiche/http2/adapter/chunked_buffer.h b/quiche/http2/adapter/chunked_buffer.h index 03ed189..86f3249 100644 --- a/quiche/http2/adapter/chunked_buffer.h +++ b/quiche/http2/adapter/chunked_buffer.h
@@ -21,6 +21,29 @@ void Append(absl::string_view data); void Append(std::unique_ptr<char[]> data, size_t size); + // A region of memory into which data may be written by the caller. + class AppendRegion { + public: + // Data may be written here. + char* data; + // At most `size` bytes may be written at the address in `data`. + const size_t size; + // Once bytes are written to `data`, this member should be updated. + size_t written; + + // Updates the state in `parent` once the write operation is complete. + ~AppendRegion(); + + private: + friend class ChunkedBuffer; + + AppendRegion(char* d, size_t s, size_t w, ChunkedBuffer* p) + : data(d), size(s), written(w), parent(p) {} + + ChunkedBuffer* const parent; + }; + AppendRegion GetAppendRegion(); + // Reads data from the buffer non-destructively. absl::string_view GetPrefix() const; std::vector<absl::string_view> Read() const; @@ -32,6 +55,8 @@ // Returns true iff the buffer contains no data to read. bool Empty() const; + size_t TotalSize() const { return total_size_; } + private: static constexpr size_t kDefaultChunkSize = 1024; @@ -75,6 +100,7 @@ void TrimFirstChunk(); quiche::QuicheCircularDeque<Chunk> chunks_; + size_t total_size_ = 0; }; } // namespace adapter
diff --git a/quiche/http2/adapter/chunked_buffer_test.cc b/quiche/http2/adapter/chunked_buffer_test.cc index 091b5f2..6a90fbe 100644 --- a/quiche/http2/adapter/chunked_buffer_test.cc +++ b/quiche/http2/adapter/chunked_buffer_test.cc
@@ -3,6 +3,7 @@ #include <algorithm> #include <initializer_list> #include <memory> +#include <string> #include <utility> #include "absl/strings/str_join.h" @@ -73,11 +74,15 @@ ChunkedBuffer buffer; auto data_and_size = MakeDataAndSize(kLoremIpsum); buffer.Append(std::move(data_and_size.data), data_and_size.size); + EXPECT_EQ(kLoremIpsum.size(), buffer.TotalSize()); buffer.RemovePrefix(6); EXPECT_THAT(buffer.GetPrefix(), testing::StartsWith("ipsum")); + EXPECT_EQ(kLoremIpsum.size() - 6, buffer.TotalSize()); buffer.RemovePrefix(20); EXPECT_THAT(buffer.GetPrefix(), testing::StartsWith(", consectetur")); + EXPECT_EQ(kLoremIpsum.size() - 26, buffer.TotalSize()); buffer.Append(" Anday igpay atinlay!"); + EXPECT_EQ(kLoremIpsum.size() - 26 + 21, buffer.TotalSize()); const std::initializer_list<absl::string_view> parts = { kLoremIpsum.substr(26), " Anday igpay atinlay!"}; EXPECT_EQ(absl::StrJoin(parts, ""), absl::StrJoin(buffer.Read(), "")); @@ -103,6 +108,71 @@ EXPECT_TRUE(buffer.Empty()); } +TEST(ChunkedBufferTest, GetAppendRegion) { + ChunkedBuffer buffer; + constexpr absl::string_view kFirstAppendText = "Lorem ipsum"; + { + auto region = buffer.GetAppendRegion(); + ASSERT_GE(region.size, kFirstAppendText.size()); + memcpy(region.data, kFirstAppendText.data(), kFirstAppendText.size()); + region.written = kFirstAppendText.size(); + } + EXPECT_EQ(kFirstAppendText, buffer.GetPrefix()); + EXPECT_EQ(kFirstAppendText.size(), buffer.TotalSize()); + + constexpr absl::string_view kSecondAppendText = " dolor sit amet, "; + { + auto region = buffer.GetAppendRegion(); + ASSERT_GE(region.size, kSecondAppendText.size()); + memcpy(region.data, kSecondAppendText.data(), kSecondAppendText.size()); + region.written = kSecondAppendText.size(); + } + EXPECT_EQ(kFirstAppendText.size() + kSecondAppendText.size(), + buffer.TotalSize()); + EXPECT_EQ(absl::StrCat(kFirstAppendText, kSecondAppendText), + buffer.GetPrefix()); +} + +TEST(ChunkedBufferTest, GetAppendRegionAcrossChunks) { + ChunkedBuffer buffer; + { + auto region = buffer.GetAppendRegion(); + ASSERT_GE(region.size, 1024); + const std::string first_part(1024, 'a'); + memcpy(region.data, first_part.data(), 1024); + region.written = 1024; + } + EXPECT_EQ(1024, buffer.TotalSize()); + { + auto region = buffer.GetAppendRegion(); + ASSERT_GE(region.size, 1024); + const std::string second_part(10, 'b'); + memcpy(region.data, second_part.data(), 10); + region.written = 10; + } + EXPECT_EQ(1034, buffer.TotalSize()); + const std::string expected = + absl::StrCat(std::string(1024, 'a'), std::string(10, 'b')); + EXPECT_EQ(expected, absl::StrJoin(buffer.Read(), "")); +} + +TEST(ChunkedBufferTest, GetAppendRegionThenAppend) { + ChunkedBuffer buffer; + constexpr absl::string_view kFirstAppendText = "Lorem ipsum"; + { + auto region = buffer.GetAppendRegion(); + ASSERT_GE(region.size, kFirstAppendText.size()); + memcpy(region.data, kFirstAppendText.data(), kFirstAppendText.size()); + region.written = kFirstAppendText.size(); + } + constexpr absl::string_view kSecondAppendText = " dolor sit amet, "; + buffer.Append(kSecondAppendText); + EXPECT_EQ(kFirstAppendText.size() + kSecondAppendText.size(), + buffer.TotalSize()); + EXPECT_EQ(absl::StrCat(kFirstAppendText, kSecondAppendText), + buffer.GetPrefix()); +} + } // namespace } // namespace adapter } // namespace http2
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index 7266ef2..88eab8e 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -11,8 +11,8 @@ #include <vector> #include "absl/cleanup/cleanup.h" -#include "absl/memory/memory.h" #include "absl/strings/escaping.h" +#include "quiche/http2/adapter/chunked_buffer.h" #include "quiche/http2/adapter/header_validator.h" #include "quiche/http2/adapter/http2_protocol.h" #include "quiche/http2/adapter/http2_util.h" @@ -953,21 +953,30 @@ std::unique_ptr<MetadataSource> source) { const uint32_t max_payload_size = std::min(kMaxAllowedMetadataFrameSize, max_frame_payload_); - auto payload_buffer = std::make_unique<uint8_t[]>(max_payload_size); while (true) { - auto [written, end_metadata] = - source->Pack(payload_buffer.get(), max_payload_size); - if (written < 0) { - // Unable to pack any metadata. - return; + ChunkedBuffer payload_buffer; + int64_t written = 0; + bool end_metadata = false; + while (payload_buffer.TotalSize() < max_payload_size) { + auto region = payload_buffer.GetAppendRegion(); + // Packs as much of the payload as will fit into `region`. + std::tie(written, end_metadata) = source->Pack( + reinterpret_cast<uint8_t*>(region.data), + std::min(region.size, max_payload_size - payload_buffer.TotalSize())); + if (written < 0) { + // Unable to pack any metadata. + return; + } + region.written = written; + if (end_metadata) { + break; + } } - QUICHE_DCHECK_LE(static_cast<size_t>(written), max_payload_size); - auto payload = absl::string_view( - reinterpret_cast<const char*>(payload_buffer.get()), written); + std::string payload = absl::StrJoin(payload_buffer.Read(), ""); EnqueueFrame(std::make_unique<spdy::SpdyUnknownIR>( stream_id, kMetadataFrameType, end_metadata ? kMetadataEndFlag : 0u, - std::string(payload))); + std::move(payload))); if (end_metadata) { return; } @@ -977,21 +986,33 @@ void OgHttp2Session::SerializeMetadata(Http2StreamId stream_id) { const uint32_t max_payload_size = std::min(kMaxAllowedMetadataFrameSize, max_frame_payload_); - auto payload_buffer = std::make_unique<uint8_t[]>(max_payload_size); while (true) { - auto [written, end_metadata] = visitor_.PackMetadataForStream( - stream_id, payload_buffer.get(), max_payload_size); - if (written < 0) { - // Unable to pack any metadata. - return; + ChunkedBuffer payload_buffer; + int64_t written = 0; + bool end_metadata = false; + + while (payload_buffer.TotalSize() < max_payload_size) { + auto region = payload_buffer.GetAppendRegion(); + // Packs as much of the payload as will fit into `region`. + std::tie(written, end_metadata) = visitor_.PackMetadataForStream( + stream_id, reinterpret_cast<uint8_t*>(region.data), + std::min(region.size, max_payload_size - payload_buffer.TotalSize())); + if (written < 0) { + // Unable to pack any metadata. + return; + } + region.written = written; + if (end_metadata) { + break; + } } - QUICHE_DCHECK_LE(static_cast<size_t>(written), max_payload_size); - auto payload = absl::string_view( - reinterpret_cast<const char*>(payload_buffer.get()), written); + QUICHE_DCHECK_LE(static_cast<size_t>(payload_buffer.TotalSize()), + max_payload_size); + std::string payload = absl::StrJoin(payload_buffer.Read(), ""); EnqueueFrame(std::make_unique<spdy::SpdyUnknownIR>( stream_id, kMetadataFrameType, end_metadata ? kMetadataEndFlag : 0u, - std::string(payload))); + std::move(payload))); if (end_metadata) { return; }
diff --git a/quiche/http2/adapter/oghttp2_session_test.cc b/quiche/http2/adapter/oghttp2_session_test.cc index 4afcf1a..8e637f0 100644 --- a/quiche/http2/adapter/oghttp2_session_test.cc +++ b/quiche/http2/adapter/oghttp2_session_test.cc
@@ -29,6 +29,17 @@ WINDOW_UPDATE, }; +// A metadata source that always fails to pack data. +class FailingMetadataSource : public MetadataSource { + public: + size_t NumFrames(size_t /*max_frame_size*/) const override { return 1; } + std::pair<int64_t, bool> Pack(uint8_t* /*dest*/, + size_t /*dest_len*/) override { + return {-1, false}; + } + void OnFailure() override {} +}; + } // namespace TEST(OgHttp2SessionTest, ClientConstruction) { @@ -1386,6 +1397,29 @@ EXPECT_EQ(session.GetReceiveWindowSize(), kInitialFlowControlWindowSize); } +TEST(OgHttp2SessionTest, FailingMetadataSourceDoesNotSend) { + TestVisitor visitor; + OgHttp2Session::Options options; + options.perspective = Perspective::kClient; + OgHttp2Session session(visitor, options); + + // A failing metadata source should not cause any METADATA frames to be sent. + auto failing_source = std::make_unique<FailingMetadataSource>(); + session.SubmitMetadata(1, std::move(failing_source)); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(16 /*kMetadataFrameType*/, _, _, _)) + .Times(0); + + EXPECT_EQ(0, session.Send()); + absl::string_view serialized = visitor.data(); + EXPECT_THAT(serialized, + testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + serialized.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(serialized, EqualsFrames({SpdyFrameType::SETTINGS})); +} + } // namespace test } // namespace adapter } // namespace http2