Do not track unacknowledged references on the encoder stream in QpackBlockingManager.
This CL essentially reverts change 263116461, which was an oversight on my part.
The reason is that unacknowledged references only have to be tracked so that no
entry with unacknowledged references is evicted from the dynamic table.
However, eviction is caused by an insertion on the encoder stream, and by that
time that is processed, it is guaranteed that every reference on the encoder
stream sent before that insertion is already processed by the decoder. (And of
course there can not be references on the encoder stream after the entry is
evicted.) So references on the encoder stream do not matter, only references on
other streams.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 275309366
Change-Id: Id3e4845f76815bc228b567fb55124a5d771843d9
diff --git a/quic/core/qpack/qpack_blocking_manager.cc b/quic/core/qpack/qpack_blocking_manager.cc
index ffec172..3544f1c 100644
--- a/quic/core/qpack/qpack_blocking_manager.cc
+++ b/quic/core/qpack/qpack_blocking_manager.cc
@@ -24,7 +24,7 @@
const uint64_t required_index_count = RequiredInsertCount(indices);
if (known_received_count_ < required_index_count) {
- IncreaseKnownReceivedCountTo(required_index_count);
+ known_received_count_ = required_index_count;
}
DecreaseReferenceCounts(indices);
@@ -56,7 +56,7 @@
return false;
}
- IncreaseKnownReceivedCountTo(known_received_count_ + increment);
+ known_received_count_ += increment;
return true;
}
@@ -68,16 +68,6 @@
header_blocks_[stream_id].push_back(std::move(indices));
}
-void QpackBlockingManager::OnReferenceSentOnEncoderStream(
- uint64_t inserted_index,
- uint64_t referred_index) {
- auto result = unacked_encoder_stream_references_.insert(
- {inserted_index, referred_index});
- // Each dynamic table entry can refer to at most one |referred_index|.
- DCHECK(result.second);
- IncreaseReferenceCounts({referred_index});
-}
-
bool QpackBlockingManager::blocking_allowed_on_stream(
QuicStreamId stream_id,
uint64_t maximum_blocked_streams) const {
@@ -141,26 +131,6 @@
return *indices.rbegin() + 1;
}
-void QpackBlockingManager::IncreaseKnownReceivedCountTo(
- uint64_t new_known_received_count) {
- DCHECK_GT(new_known_received_count, known_received_count_);
-
- known_received_count_ = new_known_received_count;
-
- // Remove referred indices with key less than new Known Received Count from
- // |unacked_encoder_stream_references_| and |entry_reference_counts_|.
- IndexSet acknowledged_references;
- auto it = unacked_encoder_stream_references_.begin();
- while (it != unacked_encoder_stream_references_.end() &&
- it->first < known_received_count_) {
- acknowledged_references.insert(it->second);
- ++it;
- }
- unacked_encoder_stream_references_.erase(
- unacked_encoder_stream_references_.begin(), it);
- DecreaseReferenceCounts(acknowledged_references);
-}
-
void QpackBlockingManager::IncreaseReferenceCounts(const IndexSet& indices) {
for (const uint64_t index : indices) {
auto it = entry_reference_counts_.lower_bound(index);
diff --git a/quic/core/qpack/qpack_blocking_manager.h b/quic/core/qpack/qpack_blocking_manager.h
index 60f7db7..6d2df9c 100644
--- a/quic/core/qpack/qpack_blocking_manager.h
+++ b/quic/core/qpack/qpack_blocking_manager.h
@@ -48,12 +48,6 @@
// entries with |indices|. |indices| must not be empty.
void OnHeaderBlockSent(QuicStreamId stream_id, IndexSet indices);
- // Called when sending Insert With Name Reference or Duplicate instruction on
- // encoder stream, inserting entry |inserted_index| referring to
- // |referred_index|.
- void OnReferenceSentOnEncoderStream(uint64_t inserted_index,
- uint64_t referred_index);
-
// Returns true if sending blocking references on stream |stream_id| would not
// increase the total number of blocked streams above
// |maximum_blocked_streams|. Note that if |stream_id| is already blocked
@@ -85,11 +79,6 @@
using HeaderBlocksForStream = std::list<IndexSet>;
using HeaderBlocks = QuicUnorderedMap<QuicStreamId, HeaderBlocksForStream>;
- // Increases |known_received_count_| to |new_known_received_count|, which must
- // me larger than |known_received_count_|. Removes acknowledged references
- // from |unacked_encoder_stream_references_|.
- void IncreaseKnownReceivedCountTo(uint64_t new_known_received_count);
-
// Increase or decrease the reference count for each index in |indices|.
void IncreaseReferenceCounts(const IndexSet& indices);
void DecreaseReferenceCounts(const IndexSet& indices);
@@ -98,13 +87,7 @@
// Must not contain a stream id with an empty queue.
HeaderBlocks header_blocks_;
- // Unacknowledged references on the encoder stream.
- // The key is the absolute index of the inserted entry,
- // the mapped value is the absolute index of the entry referred.
- std::map<uint64_t, uint64_t> unacked_encoder_stream_references_;
-
- // Number of references in |header_blocks_| and
- // |unacked_encoder_stream_references_| for each entry index.
+ // Number of references in |header_blocks_| for each entry index.
std::map<uint64_t, uint64_t> entry_reference_counts_;
uint64_t known_received_count_;
diff --git a/quic/core/qpack/qpack_blocking_manager_test.cc b/quic/core/qpack/qpack_blocking_manager_test.cc
index 64bfb97..d92dda5 100644
--- a/quic/core/qpack/qpack_blocking_manager_test.cc
+++ b/quic/core/qpack/qpack_blocking_manager_test.cc
@@ -236,88 +236,6 @@
manager_.smallest_blocking_index());
}
-TEST_F(QpackBlockingManagerTest,
- ReferenceOnEncoderStreamUnblockedByInsertCountIncrement) {
- EXPECT_EQ(0u, manager_.known_received_count());
- EXPECT_EQ(std::numeric_limits<uint64_t>::max(),
- manager_.smallest_blocking_index());
-
- // Entry 1 refers to entry 0.
- manager_.OnReferenceSentOnEncoderStream(1, 0);
- // Entry 2 also refers to entry 0.
- manager_.OnReferenceSentOnEncoderStream(2, 0);
-
- EXPECT_EQ(0u, manager_.known_received_count());
- EXPECT_EQ(0u, manager_.smallest_blocking_index());
-
- // Acknowledging entry 1 still leaves one unacknowledged reference to entry 0.
- EXPECT_TRUE(manager_.OnInsertCountIncrement(2));
-
- EXPECT_EQ(2u, manager_.known_received_count());
- EXPECT_EQ(0u, manager_.smallest_blocking_index());
-
- // Entry 3 also refers to entry 2.
- manager_.OnReferenceSentOnEncoderStream(3, 2);
-
- EXPECT_EQ(2u, manager_.known_received_count());
- EXPECT_EQ(0u, manager_.smallest_blocking_index());
-
- // Acknowledging entry 2 removes last reference to entry 0.
- EXPECT_TRUE(manager_.OnInsertCountIncrement(1));
-
- EXPECT_EQ(3u, manager_.known_received_count());
- EXPECT_EQ(2u, manager_.smallest_blocking_index());
-
- // Acknowledging entry 4 (and implicitly 3) removes reference to entry 2.
- EXPECT_TRUE(manager_.OnInsertCountIncrement(2));
-
- EXPECT_EQ(5u, manager_.known_received_count());
- EXPECT_EQ(std::numeric_limits<uint64_t>::max(),
- manager_.smallest_blocking_index());
-}
-
-TEST_F(QpackBlockingManagerTest,
- ReferenceOnEncoderStreamUnblockedByHeaderAcknowledgement) {
- EXPECT_EQ(0u, manager_.known_received_count());
- EXPECT_EQ(std::numeric_limits<uint64_t>::max(),
- manager_.smallest_blocking_index());
-
- // Entry 1 refers to entry 0.
- manager_.OnReferenceSentOnEncoderStream(1, 0);
- // Entry 2 also refers to entry 0.
- manager_.OnReferenceSentOnEncoderStream(2, 0);
-
- EXPECT_EQ(0u, manager_.known_received_count());
- EXPECT_EQ(0u, manager_.smallest_blocking_index());
-
- // Acknowledging a header block with entries up to 1 still leave one
- // unacknowledged reference to entry 0.
- manager_.OnHeaderBlockSent(/* stream_id = */ 0, {0, 1});
- manager_.OnHeaderAcknowledgement(/* stream_id = */ 0);
-
- EXPECT_EQ(2u, manager_.known_received_count());
- EXPECT_EQ(0u, manager_.smallest_blocking_index());
-
- // Entry 3 also refers to entry 2.
- manager_.OnReferenceSentOnEncoderStream(3, 2);
-
- // Acknowledging a header block with entries up to 2 removes last reference to
- // entry 0.
- manager_.OnHeaderBlockSent(/* stream_id = */ 0, {2, 0, 2});
- manager_.OnHeaderAcknowledgement(/* stream_id = */ 0);
-
- EXPECT_EQ(3u, manager_.known_received_count());
- EXPECT_EQ(2u, manager_.smallest_blocking_index());
-
- // Acknowledging entry 4 (and implicitly 3) removes reference to entry 2.
- manager_.OnHeaderBlockSent(/* stream_id = */ 0, {1, 4, 2, 0});
- manager_.OnHeaderAcknowledgement(/* stream_id = */ 0);
-
- EXPECT_EQ(5u, manager_.known_received_count());
- EXPECT_EQ(std::numeric_limits<uint64_t>::max(),
- manager_.smallest_blocking_index());
-}
-
TEST_F(QpackBlockingManagerTest, BlockingAllowedOnStream) {
const QuicStreamId kStreamId1 = 1;
const QuicStreamId kStreamId2 = 2;
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc
index 319af82..599c575 100644
--- a/quic/core/qpack/qpack_encoder.cc
+++ b/quic/core/qpack/qpack_encoder.cc
@@ -159,8 +159,6 @@
QpackAbsoluteIndexToEncoderStreamRelativeIndex(
index, header_table_.inserted_entry_count()));
auto entry = header_table_.InsertEntry(name, value);
- blocking_manager_.OnReferenceSentOnEncoderStream(
- entry->InsertionIndex(), index);
instructions.push_back(EncodeIndexedHeaderField(
is_static, entry->InsertionIndex(), referred_indices));
smallest_blocking_index = std::min(smallest_blocking_index, index);
@@ -218,8 +216,6 @@
index, header_table_.inserted_entry_count()),
value);
auto entry = header_table_.InsertEntry(name, value);
- blocking_manager_.OnReferenceSentOnEncoderStream(
- entry->InsertionIndex(), index);
instructions.push_back(EncodeIndexedHeaderField(
is_static, entry->InsertionIndex(), referred_indices));
smallest_blocking_index = std::min(smallest_blocking_index, index);
