Add debug visitor for logging when QPACK encoder cannot insert into dynamic table or cannot refer to unacknowledged entries. gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 270715196 Change-Id: I94cece5ca466ddc58a668805e91acff70e468528
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc index 61ee8fc..bd16f36 100644 --- a/quic/core/qpack/qpack_encoder.cc +++ b/quic/core/qpack/qpack_encoder.cc
@@ -35,7 +35,8 @@ DecoderStreamErrorDelegate* decoder_stream_error_delegate) : decoder_stream_error_delegate_(decoder_stream_error_delegate), decoder_stream_receiver_(this), - maximum_blocked_streams_(0) { + maximum_blocked_streams_(0), + debug_visitor_(nullptr) { DCHECK(decoder_stream_error_delegate_); } @@ -110,6 +111,10 @@ const bool blocking_allowed = blocking_manager_.blocking_allowed_on_stream( stream_id, maximum_blocked_streams_); + // Track events for debug visitor. + bool dynamic_table_insertion_blocked = false; + bool blocked_stream_limit_exhausted = false; + for (const auto& header : ValueSplittingHeaderList(&header_list)) { // These strings are owned by |header_list|. QuicStringPiece name = header.first; @@ -131,20 +136,26 @@ break; } - if (blocking_allowed || index < known_received_count) { - if (index >= draining_index) { - // If allowed, refer to entry directly. + if (index >= draining_index) { + // If allowed, refer to entry directly. + if (!blocking_allowed && index >= known_received_count) { + blocked_stream_limit_exhausted = true; + } else { instructions.push_back( EncodeIndexedHeaderField(is_static, index, referred_indices)); smallest_blocking_index = std::min(smallest_blocking_index, index); break; } - - if (blocking_allowed && - QpackEntry::Size(name, value) <= - header_table_.MaxInsertSizeWithoutEvictingGivenEntry( - std::min(smallest_blocking_index, index))) { + } else { + // Entry is draining, needs to be duplicated. + if (!blocking_allowed) { + blocked_stream_limit_exhausted = true; + } else if (QpackEntry::Size(name, value) > + header_table_.MaxInsertSizeWithoutEvictingGivenEntry( + std::min(smallest_blocking_index, index))) { + dynamic_table_insertion_blocked = true; + } else { // If allowed, duplicate entry and refer to it. sent_byte_count += encoder_stream_sender_.SendDuplicate( QpackAbsoluteIndexToEncoderStreamRelativeIndex( @@ -196,10 +207,13 @@ break; } - if (blocking_allowed && - QpackEntry::Size(name, value) <= - header_table_.MaxInsertSizeWithoutEvictingGivenEntry( - std::min(smallest_blocking_index, index))) { + if (!blocking_allowed) { + blocked_stream_limit_exhausted = true; + } else if (QpackEntry::Size(name, value) > + header_table_.MaxInsertSizeWithoutEvictingGivenEntry( + std::min(smallest_blocking_index, index))) { + dynamic_table_insertion_blocked = true; + } else { // If allowed, insert entry with name reference and refer to it. sent_byte_count += encoder_stream_sender_.SendInsertWithNameReference( is_static, @@ -236,11 +250,14 @@ break; case QpackHeaderTable::MatchType::kNoMatch: - if (blocking_allowed && - QpackEntry::Size(name, value) <= - header_table_.MaxInsertSizeWithoutEvictingGivenEntry( - smallest_blocking_index)) { - // If allowed, insert entry and refer to it. + // If allowed, insert entry and refer to it. + if (!blocking_allowed) { + blocked_stream_limit_exhausted = true; + } else if (QpackEntry::Size(name, value) > + header_table_.MaxInsertSizeWithoutEvictingGivenEntry( + smallest_blocking_index)) { + dynamic_table_insertion_blocked = true; + } else { sent_byte_count += encoder_stream_sender_.SendInsertWithoutNameReference(name, value); @@ -255,6 +272,9 @@ } // Encode entry as string literals. + // TODO(b/112770235): Consider also adding to dynamic table to improve + // compression ratio for subsequent header blocks with peers that do not + // allow any blocked streams. instructions.push_back(EncodeLiteralHeaderField(name, value)); break; @@ -267,6 +287,11 @@ *encoder_stream_sent_byte_count = sent_byte_count; } + if (debug_visitor_) { + debug_visitor_->OnHeaderListEncoded(dynamic_table_insertion_blocked, + blocked_stream_limit_exhausted); + } + return instructions; }
diff --git a/quic/core/qpack/qpack_encoder.h b/quic/core/qpack/qpack_encoder.h index 2ffbc99..c4aaced 100644 --- a/quic/core/qpack/qpack_encoder.h +++ b/quic/core/qpack/qpack_encoder.h
@@ -46,6 +46,25 @@ virtual void OnDecoderStreamError(QuicStringPiece error_message) = 0; }; + // Interface for logging whenever dynamic table insertion is blocked and + // whenever using unacknowledged entries is not allowed because of the blocked + // stream limit. + class QUIC_EXPORT_PRIVATE DebugVisitor { + public: + virtual ~DebugVisitor() = default; + + // Called once for each header list encoded. + // |dynamic_table_insertion_blocked| it true if for any header field in this + // header list the encoder would have inserted an entry into the dynamic + // table, but could not because that insertion would have evicted a blocking + // entry. |blocked_stream_limit_exhausted| is true if for any header field + // in this header list the encoder would have referred to an unacknowledged + // entry in the dynamic table, but could not because that would have + // violated the limit on the number of blocked streams. + virtual void OnHeaderListEncoded(bool dynamic_table_insertion_blocked, + bool blocked_stream_limit_exhausted) = 0; + }; + QpackEncoder(DecoderStreamErrorDelegate* decoder_stream_error_delegate); ~QpackEncoder() override; @@ -86,6 +105,10 @@ return &decoder_stream_receiver_; } + void set_debug_visitor(DebugVisitor* debug_visitor) { + debug_visitor_ = debug_visitor; + } + private: friend class test::QpackEncoderPeer; @@ -148,6 +171,7 @@ QpackHeaderTable header_table_; uint64_t maximum_blocked_streams_; QpackBlockingManager blocking_manager_; + DebugVisitor* debug_visitor_; }; } // namespace quic
diff --git a/quic/core/qpack/qpack_encoder_test.cc b/quic/core/qpack/qpack_encoder_test.cc index b0464c8..e6ee259 100644 --- a/quic/core/qpack/qpack_encoder_test.cc +++ b/quic/core/qpack/qpack_encoder_test.cc
@@ -23,6 +23,13 @@ namespace test { namespace { +class MockDebugVisitor : public QpackEncoder::DebugVisitor { + public: + virtual ~MockDebugVisitor() = default; + + MOCK_METHOD(void, OnHeaderListEncoded, (bool, bool), (override)); +}; + class QpackEncoderTest : public QuicTest { protected: QpackEncoderTest() @@ -466,6 +473,70 @@ EXPECT_EQ(30u, QpackHeaderTablePeer::dynamic_table_capacity(header_table)); } +TEST_F(QpackEncoderTest, DebugVisitor) { + StrictMock<MockDebugVisitor> debug_visitor; + encoder_.set_debug_visitor(&debug_visitor); + encoder_.SetMaximumDynamicTableCapacity(QpackEntry::Size("foo", "bar")); + + // Set Dynamic Table Capacity instruction. + EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_)); + encoder_.SetDynamicTableCapacity(QpackEntry::Size("foo", "bar")); + + // Static table entry only, no blocking. + spdy::SpdyHeaderBlock header_list1; + header_list1[":method"] = "GET"; + EXPECT_CALL(debug_visitor, OnHeaderListEncoded( + /* dynamic_table_insertion_blocked = */ false, + /* blocked_stream_limit_exhausted = */ false)); + encoder_.EncodeHeaderList(/* stream_id = */ 1, header_list1, nullptr); + + // Insert single dynamic table entry. First stream is allowed to be blocked. + spdy::SpdyHeaderBlock header_list2; + header_list2["foo"] = "bar"; + EXPECT_CALL(debug_visitor, OnHeaderListEncoded( + /* dynamic_table_insertion_blocked = */ false, + /* blocked_stream_limit_exhausted = */ false)); + // Add entry to dynamic table. + EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_)); + encoder_.EncodeHeaderList(/* stream_id = */ 1, header_list2, nullptr); + + // First stream is blocked. Second stream is not allowed to be blocked. + // This would have been an insertion with name reference. + spdy::SpdyHeaderBlock header_list3; + header_list3["foo"] = "baz"; + EXPECT_CALL(debug_visitor, OnHeaderListEncoded( + /* dynamic_table_insertion_blocked = */ false, + /* blocked_stream_limit_exhausted = */ true)); + encoder_.EncodeHeaderList(/* stream_id = */ 2, header_list3, nullptr); + + // First stream is blocked. Second stream is not allowed to be blocked. + // This would have been an insertion without name reference. + spdy::SpdyHeaderBlock header_list4; + header_list4["bar"] = "baz"; + EXPECT_CALL(debug_visitor, OnHeaderListEncoded( + /* dynamic_table_insertion_blocked = */ false, + /* blocked_stream_limit_exhausted = */ true)); + encoder_.EncodeHeaderList(/* stream_id = */ 2, header_list4, nullptr); + + // Cannot insert into dynamic table because first entry takes up all space and + // it is blocking. This would have been an insertion with name reference. + spdy::SpdyHeaderBlock header_list5; + header_list5["foo"] = "baz"; + EXPECT_CALL(debug_visitor, OnHeaderListEncoded( + /* dynamic_table_insertion_blocked = */ true, + /* blocked_stream_limit_exhausted = */ false)); + encoder_.EncodeHeaderList(/* stream_id = */ 1, header_list5, nullptr); + + // Cannot insert into dynamic table because first entry takes up all space and + // it is blocking. This would have been an insertion without name reference. + spdy::SpdyHeaderBlock header_list6; + header_list6["bar"] = "baz"; + EXPECT_CALL(debug_visitor, OnHeaderListEncoded( + /* dynamic_table_insertion_blocked = */ true, + /* blocked_stream_limit_exhausted = */ false)); + encoder_.EncodeHeaderList(/* stream_id = */ 1, header_list6, nullptr); +} + } // namespace } // namespace test } // namespace quic