From 6d480e9d34fd7cc992ba190e966cf9eb13c2e393 Mon Sep 17 00:00:00 2001 From: dpcollins-google <40498610+dpcollins-google@users.noreply.github.com> Date: Thu, 28 Apr 2022 15:53:02 -0400 Subject: [PATCH] Restructure HPackParserTable to not use an inline table, and use a std::vector for its ring buffer instead. (#29394) * Restructure HPackParserTable to not use an inline table, and use a std::vector for its ring buffer instead. Before this change, it uses 6.2KiB by default, 2/3 of the size of grpc_chttp2_transport. https://screenshot.googleplex.com/6qdcybty2oCsGjG After this change, it uses 120 bytes https://screenshot.googleplex.com/4xGWKmZZz68VE4F This class uses up substantial amounts of memory, as it is allocated on a per-stream basis. This should reduce the size of grpc_chttp2_stream substantially It should cause a significant memory reduction, as the HPack extension table should almost never be fully populated in terms of number of entries, especially since common headers already exist in a static table and entries are highly likely to take more memory than the absolute minimum. * formatting * delete unused method * move MememtoRingBuffer * reformat --- .../chttp2/transport/hpack_parser_table.cc | 81 +++++++++++-------- .../chttp2/transport/hpack_parser_table.h | 53 +++++++----- 2 files changed, 82 insertions(+), 52 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc b/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc index 17dd0a4d2eb..261c7bd1b8c 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc @@ -20,8 +20,9 @@ #include "src/core/ext/transport/chttp2/transport/hpack_parser_table.h" -#include -#include +#include +#include +#include #include "absl/strings/str_format.h" @@ -38,29 +39,55 @@ extern grpc_core::TraceFlag grpc_http_trace; namespace grpc_core { -HPackTable::HPackTable() : static_metadata_(GetStaticMementos()) {} +void HPackTable::MementoRingBuffer::Put(Memento m) { + GPR_ASSERT(num_entries_ < max_entries_); + if (entries_.size() < max_entries_) { + ++num_entries_; + return entries_.push_back(std::move(m)); + } + size_t index = (first_entry_ + num_entries_) % max_entries_; + entries_[index] = std::move(m); + ++num_entries_; +} -HPackTable::~HPackTable() = default; +auto HPackTable::MementoRingBuffer::PopOne() -> Memento { + GPR_ASSERT(num_entries_ > 0); + size_t index = first_entry_ % max_entries_; + ++first_entry_; + --num_entries_; + return std::move(entries_[index]); +} -/* Evict one element from the table */ -void HPackTable::EvictOne() { - auto first_entry = std::move(entries_[first_entry_]); - GPR_ASSERT(first_entry.transport_size() <= mem_used_); - mem_used_ -= first_entry.transport_size(); - first_entry_ = ((first_entry_ + 1) % entries_.size()); - num_entries_--; +auto HPackTable::MementoRingBuffer::Lookup(uint32_t index) const + -> const Memento* { + if (index >= num_entries_) return nullptr; + uint32_t offset = (num_entries_ - 1u - index + first_entry_) % max_entries_; + return &entries_[offset]; } -void HPackTable::Rebuild(uint32_t new_cap) { - EntriesVec entries; - entries.resize(new_cap); +void HPackTable::MementoRingBuffer::Rebuild(uint32_t max_entries) { + if (max_entries == max_entries_) return; + std::vector entries; + entries.reserve(num_entries_); for (size_t i = 0; i < num_entries_; i++) { - entries[i] = std::move(entries_[(first_entry_ + i) % entries_.size()]); + entries.push_back( + std::move(entries_[(first_entry_ + i) % entries_.size()])); } first_entry_ = 0; entries_.swap(entries); } +HPackTable::HPackTable() : static_metadata_(GetStaticMementos()) {} + +HPackTable::~HPackTable() = default; + +/* Evict one element from the table */ +void HPackTable::EvictOne() { + auto first_entry = entries_.PopOne(); + GPR_ASSERT(first_entry.transport_size() <= mem_used_); + mem_used_ -= first_entry.transport_size(); +} + void HPackTable::SetMaxBytes(uint32_t max_bytes) { if (max_bytes_ == max_bytes) { return; @@ -90,18 +117,9 @@ grpc_error_handle HPackTable::SetCurrentTableSize(uint32_t bytes) { EvictOne(); } current_table_bytes_ = bytes; - max_entries_ = hpack_constants::EntriesForBytes(bytes); - if (max_entries_ > entries_.size()) { - Rebuild(max_entries_); - } else if (max_entries_ < entries_.size() / 3) { - // TODO(ctiller): move to resource quota system, only shrink under memory - // pressure - uint32_t new_cap = - std::max(max_entries_, static_cast(kInlineEntries)); - if (new_cap != entries_.size()) { - Rebuild(new_cap); - } - } + uint32_t new_cap = std::max(hpack_constants::EntriesForBytes(bytes), + hpack_constants::kInitialTableEntries); + entries_.Rebuild(new_cap); return GRPC_ERROR_NONE; } @@ -122,7 +140,7 @@ grpc_error_handle HPackTable::Add(Memento md) { // attempt to add an entry larger than the entire table causes // the table to be emptied of all existing entries, and results in an // empty table. - while (num_entries_) { + while (entries_.num_entries()) { EvictOne(); } return GRPC_ERROR_NONE; @@ -136,10 +154,7 @@ grpc_error_handle HPackTable::Add(Memento md) { // copy the finalized entry in mem_used_ += md.transport_size(); - entries_[(first_entry_ + num_entries_) % entries_.size()] = std::move(md); - - // update accounting values - num_entries_++; + entries_.Put(std::move(md)); return GRPC_ERROR_NONE; } @@ -225,7 +240,7 @@ GPR_ATTRIBUTE_NOINLINE HPackTable::Memento MakeMemento(size_t i) { } // namespace -const HPackTable::StaticMementos& HPackTable::GetStaticMementos() { +auto HPackTable::GetStaticMementos() -> const StaticMementos& { static const StaticMementos* const static_mementos = new StaticMementos(); return *static_mementos; } diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser_table.h b/src/core/ext/transport/chttp2/transport/hpack_parser_table.h index 5309ecad287..532ec79417e 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser_table.h +++ b/src/core/ext/transport/chttp2/transport/hpack_parser_table.h @@ -63,7 +63,7 @@ class HPackTable { grpc_error_handle Add(Memento md) GRPC_MUST_USE_RESULT; // Current entry count in the table. - uint32_t num_entries() const { return num_entries_; } + uint32_t num_entries() const { return entries_.num_entries(); } private: struct StaticMementos { @@ -72,28 +72,46 @@ class HPackTable { }; static const StaticMementos& GetStaticMementos() GPR_ATTRIBUTE_NOINLINE; - enum { kInlineEntries = hpack_constants::kInitialTableEntries }; - using EntriesVec = absl::InlinedVector; + class MementoRingBuffer { + public: + // Rebuild this buffer with a new max_entries_ size. + void Rebuild(uint32_t max_entries); + + // Put a new memento. + // REQUIRES: num_entries < max_entries + void Put(Memento m); + + // Pop the oldest memento. + // REQUIRES: num_entries > 0 + Memento PopOne(); + + // Lookup the entry at index, or return nullptr if none exists. + const Memento* Lookup(uint32_t index) const; + + uint32_t max_entries() const { return max_entries_; } + uint32_t num_entries() const { return num_entries_; } + + private: + // The index of the first entry in the buffer. May be greater than + // max_entries_, in which case a wraparound has occurred. + uint32_t first_entry_ = 0; + // How many entries are in the table. + uint32_t num_entries_ = 0; + // Maximum number of entries we could possibly fit in the table, given + // defined overheads. + uint32_t max_entries_ = hpack_constants::kInitialTableEntries; + + std::vector entries_; + }; const Memento* LookupDynamic(uint32_t index) const { // Not static - find the value in the list of valid entries const uint32_t tbl_index = index - (hpack_constants::kLastStaticEntry + 1); - if (tbl_index < num_entries_) { - uint32_t offset = - (num_entries_ - 1u - tbl_index + first_entry_) % entries_.size(); - return &entries_[offset]; - } - // Invalid entry: return error - return nullptr; + return entries_.Lookup(tbl_index); } void EvictOne(); - void Rebuild(uint32_t new_cap); - // The first used entry in ents. - uint32_t first_entry_ = 0; - // How many entries are in the table. - uint32_t num_entries_ = 0; // The amount of memory used by the table, according to the hpack algorithm uint32_t mem_used_ = 0; // The max memory allowed to be used by the table, according to the hpack @@ -101,11 +119,8 @@ class HPackTable { uint32_t max_bytes_ = hpack_constants::kInitialTableSize; // The currently agreed size of the table, according to the hpack algorithm. uint32_t current_table_bytes_ = hpack_constants::kInitialTableSize; - // Maximum number of entries we could possibly fit in the table, given defined - // overheads. - uint32_t max_entries_ = hpack_constants::kInitialTableEntries; // HPack table entries - EntriesVec entries_{hpack_constants::kInitialTableEntries}; + MementoRingBuffer entries_; // Mementos for static data const StaticMementos& static_metadata_; };