Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize QuicStreamSetUpdateMaxStreams #4852

Merged
merged 11 commits into from
Mar 3, 2025
Merged

Conversation

guhetier
Copy link
Contributor

@guhetier guhetier commented Feb 25, 2025

Description

Store new streams that are blocked by Stream Id flow control in a sorted list (instead of the hash table containing all streams) to improve the performances of QuicStreamSetUpdateMaxStreams.

It will now be enough to iterate over a subset of this list instead of iterating over the entirety of opened streams when receiving a "MAX_STREAMS" frame increasing the limit.

Closes #3538

Testing

Already covered by existing tests.

Documentation

In code documentation added.

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 96.49123% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.55%. Comparing base (fec5ce3) to head (f20c913).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/core/stream_set.c 96.46% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4852      +/-   ##
==========================================
+ Coverage   85.93%   86.55%   +0.62%     
==========================================
  Files          56       56              
  Lines       17630    17685      +55     
==========================================
+ Hits        15150    15308     +158     
+ Misses       2480     2377     -103     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@guhetier guhetier force-pushed the guhetier/waiting_stream_list branch from bb0fac7 to 248543d Compare February 27, 2025 01:56
@guhetier guhetier marked this pull request as ready for review February 27, 2025 01:56
@guhetier guhetier requested a review from a team as a code owner February 27, 2025 01:56
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Just a few (mostly nit) comments.

@nibanks nibanks added Area: Performance Area: Core Related to the shared, core protocol logic labels Mar 3, 2025
nibanks
nibanks previously approved these changes Mar 3, 2025
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@guhetier guhetier merged commit 618c860 into main Mar 3, 2025
397 checks passed
@guhetier guhetier deleted the guhetier/waiting_stream_list branch March 3, 2025 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core Related to the shared, core protocol logic Area: Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Perf of QuicStreamSetUpdateMaxStreams
2 participants