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