Prepare QpackHeaderTable::InsertEntry() for non-stable container.
My plan is to split QpackHeaderTable: the decoder does not need
FindHeaderField(), or static_index_, static_name_index, dynamic_index_,
dynamic_index_name_ methods, therefore it does not need iterator stability for
the underlying container, but it needs random access. QuicCircularDeque would
be a good choice. This CL prepares InsertEntry() for that.
Since copy is done in a separate step before insertion, now it is safe to evict
before insertion, potentially saving on a little bit of memory.
PiperOrigin-RevId: 365609653
Change-Id: I55b6a95168d7a6bca32b9f35c2bf8d3c08472635
diff --git a/quic/core/qpack/qpack_header_table.cc b/quic/core/qpack/qpack_header_table.cc
index bd6c6d0..da3636d 100644
--- a/quic/core/qpack/qpack_header_table.cc
+++ b/quic/core/qpack/qpack_header_table.cc
@@ -103,35 +103,43 @@
QUICHE_DCHECK(EntryFitsDynamicTableCapacity(name, value));
const uint64_t index = dropped_entry_count_ + dynamic_entries_.size();
- dynamic_entries_.push_back({name, value});
- QpackEntry* const new_entry = &dynamic_entries_.back();
- // Evict entries after inserting the new entry instead of before
- // in order to avoid invalidating |name| and |value|.
- dynamic_table_size_ += QpackEntry::Size(name, value);
- EvictDownToCurrentCapacity();
+ // Copy name and value before modifying the container, because evicting
+ // entries or even inserting a new one might invalidate |name| or |value| if
+ // they point to an entry.
+ QpackEntry new_entry((std::string(name)), (std::string(value)));
+ const size_t entry_size = new_entry.Size();
- auto index_result = dynamic_index_.insert(std::make_pair(
- QpackLookupEntry{new_entry->name(), new_entry->value()}, index));
+ EvictDownToCapacity(dynamic_table_capacity_ - entry_size);
+
+ dynamic_table_size_ += entry_size;
+ dynamic_entries_.push_back(std::move(new_entry));
+
+ // Make name and value point to the new entry.
+ name = dynamic_entries_.back().name();
+ value = dynamic_entries_.back().value();
+
+ auto index_result = dynamic_index_.insert(
+ std::make_pair(QpackLookupEntry{name, value}, index));
if (!index_result.second) {
// An entry with the same name and value already exists. It needs to be
// replaced, because |dynamic_index_| tracks the most recent entry for a
// given name and value.
QUICHE_DCHECK_GT(index, index_result.first->second);
dynamic_index_.erase(index_result.first);
- auto result = dynamic_index_.insert(std::make_pair(
- QpackLookupEntry{new_entry->name(), new_entry->value()}, index));
+ auto result = dynamic_index_.insert(
+ std::make_pair(QpackLookupEntry{name, value}, index));
QUICHE_CHECK(result.second);
}
- auto name_result = dynamic_name_index_.insert({new_entry->name(), index});
+ auto name_result = dynamic_name_index_.insert({name, index});
if (!name_result.second) {
// An entry with the same name already exists. It needs to be replaced,
// because |dynamic_name_index_| tracks the most recent entry for a given
// name.
QUICHE_DCHECK_GT(index, name_result.first->second);
dynamic_name_index_.erase(name_result.first);
- auto result = dynamic_name_index_.insert({new_entry->name(), index});
+ auto result = dynamic_name_index_.insert({name, index});
QUICHE_CHECK(result.second);
}
@@ -179,7 +187,7 @@
}
dynamic_table_capacity_ = capacity;
- EvictDownToCurrentCapacity();
+ EvictDownToCapacity(capacity);
QUICHE_DCHECK_LE(dynamic_table_size_, dynamic_table_capacity_);
@@ -245,8 +253,8 @@
return entry_index;
}
-void QpackHeaderTable::EvictDownToCurrentCapacity() {
- while (dynamic_table_size_ > dynamic_table_capacity_) {
+void QpackHeaderTable::EvictDownToCapacity(uint64_t capacity) {
+ while (dynamic_table_size_ > capacity) {
QUICHE_DCHECK(!dynamic_entries_.empty());
QpackEntry* const entry = &dynamic_entries_.front();
diff --git a/quic/core/qpack/qpack_header_table.h b/quic/core/qpack/qpack_header_table.h
index 6a5e15a..a5d6c5d 100644
--- a/quic/core/qpack/qpack_header_table.h
+++ b/quic/core/qpack/qpack_header_table.h
@@ -83,8 +83,12 @@
absl::string_view value) const;
// Inserts (name, value) into the dynamic table. Entry must not be larger
- // than the capacity of the dynamic table. May evict entries. Returns the
- // absolute index of the inserted dynamic table entry.
+ // than the capacity of the dynamic table. May evict entries. |name| and
+ // |value| are copied first, therefore it is safe for them to point to an
+ // entry in the dynamic table, even if it is about to be evicted, or even if
+ // the underlying container might move entries around when resizing for
+ // insertion.
+ // Returns the absolute index of the inserted dynamic table entry.
uint64_t InsertEntry(absl::string_view name, absl::string_view value);
// Returns the size of the largest entry that could be inserted into the
@@ -156,8 +160,8 @@
friend class test::QpackHeaderTablePeer;
// Evict entries from the dynamic table until table size is less than or equal
- // to current value of |dynamic_table_capacity_|.
- void EvictDownToCurrentCapacity();
+ // to |capacity|.
+ void EvictDownToCapacity(uint64_t capacity);
// Static Table
