QPACK: look up stream references without inserting - #13628
Draft
brbzull0 wants to merge 1 commit into
Draft
Conversation
_update_largest_known_received_index_by_stream_id() and _update_reference_counts() read this->_references[stream_id], so std::map::operator[] default-inserts an entry for any stream id not already in the map. Both callers erase that entry two statements later, so behaviour is unchanged, but every Header Acknowledgement or Stream Cancellation naming an untracked stream still costs a map node allocation and free. Use find() with an early return instead.
Contributor
Author
|
[approve ci autest 2] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
QPACK::_update_largest_known_received_index_by_stream_id()andQPACK::_update_reference_counts()both readthis->_references[stream_id](
src/proxy/http3/QPACK.cc:1020,:1033).std::map::operator[]inserts adefault-constructed element when the key is absent, so using it for a read
inserts an entry for any stream id that is not already tracked.
stream_idhere comes from_read_header_acknowledgement()/_read_stream_cancellation(), i.e. straight off the peer's QPACK decoderstream, so the key is whatever the peer sent.
This is not a behaviour change. Both call sites
(
_on_decoder_stream_read_ready(),QPACK.cc:1120-1122and:1128-1129) callthis->_references.erase(stream_id)unconditionally two statements later,inside the same
if (... >= 0)block with no early exit in between. And becauseEntryReferenceis an aggregate with no user-provided constructor,operator[]value-initializes it, sosmallestandlargestare both0:largest > _largest_known_received_indexis never true for an unsigned0, andif (smallest)never fires. The inserted entry is therefore inert and thenremoved.
What it does change is cost and clarity: each Header Acknowledgement or Stream
Cancellation naming an untracked stream currently allocates and frees a
std::mapnode for nothing, andoperator[]-as-a-read is easy to misread asharmless when it is the reason the
erase()calls are load-bearing. Replacingit with
find()plus an early return removes both.Test
No new test. The change is observationally equivalent by the reasoning above, so
there is no behaviour for a regression test to pin — a test asserting the
current outputs would pass with or without the patch.
Run as a no-regression check:
h3_proxy_verifier,h3_python_clientandh3_stream_lifetimeautests (3/3 pass), which exercise real QPACK headerexchange including decoder-stream instructions.