Return absolute index instead of QpackEntry* from QpackHeaderTable::InsertEntry().
The immediate goal is to eliminate uses of InsertionIndex(). The long-term goal
is to eliminate the need for interator stability on the container that holds
QpackEntries.
PiperOrigin-RevId: 363000091
Change-Id: Ic1e9ce61d4e3b263687ca854b1bd400c288b7368
diff --git a/quic/core/qpack/qpack_decoder.cc b/quic/core/qpack/qpack_decoder.cc
index c1ac28c..b992616 100644
--- a/quic/core/qpack/qpack_decoder.cc
+++ b/quic/core/qpack/qpack_decoder.cc
@@ -81,11 +81,12 @@
return;
}
- entry = header_table_.InsertEntry(entry->name(), value);
- if (!entry) {
+ if (!header_table_.EntryFitsDynamicTableCapacity(entry->name(), value)) {
OnErrorDetected(QUIC_QPACK_ENCODER_STREAM_ERROR_INSERTING_STATIC,
"Error inserting entry with name reference.");
+ return;
}
+ header_table_.InsertEntry(entry->name(), value);
return;
}
@@ -104,20 +105,22 @@
"Dynamic table entry not found.");
return;
}
- entry = header_table_.InsertEntry(entry->name(), value);
- if (!entry) {
+ if (!header_table_.EntryFitsDynamicTableCapacity(entry->name(), value)) {
OnErrorDetected(QUIC_QPACK_ENCODER_STREAM_ERROR_INSERTING_DYNAMIC,
"Error inserting entry with name reference.");
+ return;
}
+ header_table_.InsertEntry(entry->name(), value);
}
void QpackDecoder::OnInsertWithoutNameReference(absl::string_view name,
absl::string_view value) {
- const QpackEntry* entry = header_table_.InsertEntry(name, value);
- if (!entry) {
+ if (!header_table_.EntryFitsDynamicTableCapacity(name, value)) {
OnErrorDetected(QUIC_QPACK_ENCODER_STREAM_ERROR_INSERTING_LITERAL,
"Error inserting literal entry.");
+ return;
}
+ header_table_.InsertEntry(name, value);
}
void QpackDecoder::OnDuplicate(uint64_t index) {
@@ -136,13 +139,13 @@
"Dynamic table entry not found.");
return;
}
- entry = header_table_.InsertEntry(entry->name(), entry->value());
- if (!entry) {
- // InsertEntry() can only fail if entry is larger then dynamic table
- // capacity, but that is impossible since entry was retrieved from the
- // dynamic table.
+ if (!header_table_.EntryFitsDynamicTableCapacity(entry->name(),
+ entry->value())) {
+ // This is impossible since entry was retrieved from the dynamic table.
OnErrorDetected(QUIC_INTERNAL_ERROR, "Error inserting duplicate entry.");
+ return;
}
+ header_table_.InsertEntry(entry->name(), entry->value());
}
void QpackDecoder::OnSetDynamicTableCapacity(uint64_t capacity) {
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc
index 8138266..de42996 100644
--- a/quic/core/qpack/qpack_encoder.cc
+++ b/quic/core/qpack/qpack_encoder.cc
@@ -156,9 +156,9 @@
encoder_stream_sender_.SendDuplicate(
QpackAbsoluteIndexToEncoderStreamRelativeIndex(
index, header_table_.inserted_entry_count()));
- auto entry = header_table_.InsertEntry(name, value);
+ uint64_t new_index = header_table_.InsertEntry(name, value);
instructions.push_back(EncodeIndexedHeaderField(
- is_static, entry->InsertionIndex(), referred_indices));
+ is_static, new_index, referred_indices));
smallest_blocking_index = std::min(smallest_blocking_index, index);
header_table_.set_dynamic_table_entry_referenced();
@@ -184,12 +184,11 @@
// If allowed, insert entry into dynamic table and refer to it.
encoder_stream_sender_.SendInsertWithNameReference(is_static, index,
value);
- auto entry = header_table_.InsertEntry(name, value);
+ uint64_t new_index = header_table_.InsertEntry(name, value);
instructions.push_back(EncodeIndexedHeaderField(
- /* is_static = */ false, entry->InsertionIndex(),
- referred_indices));
- smallest_blocking_index = std::min<uint64_t>(
- smallest_blocking_index, entry->InsertionIndex());
+ /* is_static = */ false, new_index, referred_indices));
+ smallest_blocking_index =
+ std::min<uint64_t>(smallest_blocking_index, new_index);
break;
}
@@ -214,9 +213,9 @@
QpackAbsoluteIndexToEncoderStreamRelativeIndex(
index, header_table_.inserted_entry_count()),
value);
- auto entry = header_table_.InsertEntry(name, value);
- instructions.push_back(EncodeIndexedHeaderField(
- is_static, entry->InsertionIndex(), referred_indices));
+ uint64_t new_index = header_table_.InsertEntry(name, value);
+ instructions.push_back(
+ EncodeIndexedHeaderField(is_static, new_index, referred_indices));
smallest_blocking_index = std::min(smallest_blocking_index, index);
header_table_.set_dynamic_table_entry_referenced();
@@ -253,12 +252,11 @@
dynamic_table_insertion_blocked = true;
} else {
encoder_stream_sender_.SendInsertWithoutNameReference(name, value);
- auto entry = header_table_.InsertEntry(name, value);
+ uint64_t new_index = header_table_.InsertEntry(name, value);
instructions.push_back(EncodeIndexedHeaderField(
- /* is_static = */ false, entry->InsertionIndex(),
- referred_indices));
- smallest_blocking_index = std::min<uint64_t>(smallest_blocking_index,
- entry->InsertionIndex());
+ /* is_static = */ false, new_index, referred_indices));
+ smallest_blocking_index =
+ std::min<uint64_t>(smallest_blocking_index, new_index);
break;
}
diff --git a/quic/core/qpack/qpack_header_table.cc b/quic/core/qpack/qpack_header_table.cc
index 6171d84..e8d4c24 100644
--- a/quic/core/qpack/qpack_header_table.cc
+++ b/quic/core/qpack/qpack_header_table.cc
@@ -96,12 +96,15 @@
return MatchType::kNoMatch;
}
-const QpackEntry* QpackHeaderTable::InsertEntry(absl::string_view name,
- absl::string_view value) {
- const uint64_t entry_size = QpackEntry::Size(name, value);
- if (entry_size > dynamic_table_capacity_) {
- return nullptr;
- }
+bool QpackHeaderTable::EntryFitsDynamicTableCapacity(
+ absl::string_view name,
+ absl::string_view value) const {
+ return QpackEntry::Size(name, value) <= dynamic_table_capacity_;
+}
+
+uint64_t QpackHeaderTable::InsertEntry(absl::string_view name,
+ absl::string_view value) {
+ QUICHE_DCHECK(EntryFitsDynamicTableCapacity(name, value));
const uint64_t index = dropped_entry_count_ + dynamic_entries_.size();
dynamic_entries_.push_back({name, value, /* is_static = */ false, index});
@@ -109,7 +112,7 @@
// Evict entries after inserting the new entry instead of before
// in order to avoid invalidating |name| and |value|.
- dynamic_table_size_ += entry_size;
+ dynamic_table_size_ += QpackEntry::Size(name, value);
EvictDownToCurrentCapacity();
auto index_result = dynamic_index_.insert(new_entry);
@@ -147,7 +150,7 @@
observer->OnInsertCountReachedThreshold();
}
- return new_entry;
+ return index;
}
uint64_t QpackHeaderTable::MaxInsertSizeWithoutEvictingGivenEntry(
diff --git a/quic/core/qpack/qpack_header_table.h b/quic/core/qpack/qpack_header_table.h
index 4f0a1b5..0e9a5a5 100644
--- a/quic/core/qpack/qpack_header_table.h
+++ b/quic/core/qpack/qpack_header_table.h
@@ -75,11 +75,16 @@
bool* is_static,
uint64_t* index) const;
- // Insert (name, value) into the dynamic table. May evict entries. Returns a
- // pointer to the inserted owned entry on success. Returns nullptr if entry
- // is larger than the capacity of the dynamic table.
- const QpackEntry* InsertEntry(absl::string_view name,
- absl::string_view value);
+ // Returns whether an entry with |name| and |value| has a size (including
+ // overhead) that is smaller than or equal to the capacity of the dynamic
+ // table.
+ bool EntryFitsDynamicTableCapacity(absl::string_view name,
+ 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.
+ uint64_t InsertEntry(absl::string_view name, absl::string_view value);
// Returns the size of the largest entry that could be inserted into the
// dynamic table without evicting entry |index|. |index| might be larger than
diff --git a/quic/core/qpack/qpack_header_table_test.cc b/quic/core/qpack/qpack_header_table_test.cc
index 801ab8f..0af250e 100644
--- a/quic/core/qpack/qpack_header_table_test.cc
+++ b/quic/core/qpack/qpack_header_table_test.cc
@@ -81,13 +81,13 @@
<< name << ": " << value;
}
- void InsertEntry(absl::string_view name, absl::string_view value) {
- EXPECT_TRUE(table_.InsertEntry(name, value));
+ bool EntryFitsDynamicTableCapacity(absl::string_view name,
+ absl::string_view value) const {
+ return table_.EntryFitsDynamicTableCapacity(name, value);
}
- void ExpectToFailInsertingEntry(absl::string_view name,
- absl::string_view value) {
- EXPECT_FALSE(table_.InsertEntry(name, value));
+ void InsertEntry(absl::string_view name, absl::string_view value) {
+ table_.InsertEntry(name, value);
}
bool SetDynamicTableCapacity(uint64_t capacity) {
@@ -293,9 +293,6 @@
ExpectNoMatch("foo", "bar");
ExpectMatch("baz", "qux", QpackHeaderTable::MatchType::kNameAndValue,
/* expected_is_static = */ false, 1u);
-
- // Inserting an entry that does not fit results in error.
- ExpectToFailInsertingEntry("foobar", "foobar");
}
TEST_F(QpackHeaderTableTest, EvictByUpdateTableSize) {
@@ -389,7 +386,7 @@
table.MaxInsertSizeWithoutEvictingGivenEntry(0));
const uint64_t entry_size1 = QpackEntry::Size("foo", "bar");
- EXPECT_TRUE(table.InsertEntry("foo", "bar"));
+ table.InsertEntry("foo", "bar");
EXPECT_EQ(dynamic_table_capacity - entry_size1,
table.MaxInsertSizeWithoutEvictingGivenEntry(0));
// Table can take an entry up to its capacity if all entries are allowed to be
@@ -398,7 +395,7 @@
table.MaxInsertSizeWithoutEvictingGivenEntry(1));
const uint64_t entry_size2 = QpackEntry::Size("baz", "foobar");
- EXPECT_TRUE(table.InsertEntry("baz", "foobar"));
+ table.InsertEntry("baz", "foobar");
// Table can take an entry up to its capacity if all entries are allowed to be
// evicted.
EXPECT_EQ(dynamic_table_capacity,
@@ -412,7 +409,7 @@
// Third entry evicts first one.
const uint64_t entry_size3 = QpackEntry::Size("last", "entry");
- EXPECT_TRUE(table.InsertEntry("last", "entry"));
+ table.InsertEntry("last", "entry");
EXPECT_EQ(1u, table.dropped_entry_count());
// Table can take an entry up to its capacity if all entries are allowed to be
// evicted.
@@ -497,14 +494,14 @@
EXPECT_EQ(0u, table.draining_index(1.0));
// Table with one entry.
- EXPECT_TRUE(table.InsertEntry("foo", "bar"));
+ table.InsertEntry("foo", "bar");
// Any entry can be referenced if none of the table is draining.
EXPECT_EQ(0u, table.draining_index(0.0));
// No entry can be referenced if all of the table is draining.
EXPECT_EQ(1u, table.draining_index(1.0));
// Table with two entries is at half capacity.
- EXPECT_TRUE(table.InsertEntry("foo", "bar"));
+ table.InsertEntry("foo", "bar");
// Any entry can be referenced if at most half of the table is draining,
// because current entries only take up half of total capacity.
EXPECT_EQ(0u, table.draining_index(0.0));
@@ -513,8 +510,8 @@
EXPECT_EQ(2u, table.draining_index(1.0));
// Table with four entries is full.
- EXPECT_TRUE(table.InsertEntry("foo", "bar"));
- EXPECT_TRUE(table.InsertEntry("foo", "bar"));
+ table.InsertEntry("foo", "bar");
+ table.InsertEntry("foo", "bar");
// Any entry can be referenced if none of the table is draining.
EXPECT_EQ(0u, table.draining_index(0.0));
// In a full table with identically sized entries, |draining_fraction| of all
@@ -533,6 +530,14 @@
table.reset();
}
+TEST_F(QpackHeaderTableTest, EntryFitsDynamicTableCapacity) {
+ EXPECT_TRUE(SetDynamicTableCapacity(39));
+
+ EXPECT_TRUE(EntryFitsDynamicTableCapacity("foo", "bar"));
+ EXPECT_TRUE(EntryFitsDynamicTableCapacity("foo", "bar2"));
+ EXPECT_FALSE(EntryFitsDynamicTableCapacity("foo", "bar12"));
+}
+
} // namespace
} // namespace test
} // namespace quic
