Skip to content

HTTP/3: Http3Stream pooling #34576

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

Merged
merged 4 commits into from
Jul 24, 2021
Merged

HTTP/3: Http3Stream pooling #34576

merged 4 commits into from
Jul 24, 2021

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 21, 2021

Fixes #33680

This builds on top of the new IPersistentStateFeature.

Http3Stream and many of its associated types (pipes, output producer, decoder) are now reused between requests.

In this PR I also added some basic microbenchmarks that do HTTP/3. Refactoring the test code so it is usable in microbenchmarks is the major of line changes in this PR.

I will get benchmarks comparing allocations before and after.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

Seems OK in general once the build is fixed. I'm curious to see the benchmarks.

@JamesNK JamesNK force-pushed the jamesnk/http3-http3streapooling3 branch from 4a31edd to 899e200 Compare July 21, 2021 22:01
_streamLifetimeHandler.OnStreamCreated(stream);

ThreadPool.UnsafeQueueUserWorkItem(stream, preferLocal: false);
}
else
{
var persistentStateFeature = streamContext.Features.Get<IPersistentStateFeature>();
Debug.Assert(persistentStateFeature != null, $"Required {nameof(IPersistentStateFeature)} not on stream context.");
Copy link
Member

Choose a reason for hiding this comment

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

This seems too possible to just assert given the transport is pluggable. Let's null-check and throw.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I also look forward to the benchmarks.

@JamesNK JamesNK force-pushed the jamesnk/http3-http3streapooling3 branch from 899e200 to 858d3ee Compare July 23, 2021 09:34
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

New and renamed files need MIT license headers

@JamesNK
Copy link
Member Author

JamesNK commented Jul 23, 2021

I thought I had already updated old files. Which ones still need to be updated?

Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

I thought I had already updated old files. Which ones still need to be updated?

I must have looked at this and still had an older copy of the changes cached. Sorry.

I don't see another way to remove my changes request and am signing off only on the headings.

@JamesNK
Copy link
Member Author

JamesNK commented Jul 23, 2021

Before - 300mb allocated for 10,000 requests
image

After - 50mb allocated for 10,000 requests
image

@JamesNK JamesNK enabled auto-merge (squash) July 24, 2021 00:48
@JamesNK JamesNK merged commit 0c5456a into main Jul 24, 2021
@JamesNK JamesNK deleted the jamesnk/http3-http3streapooling3 branch July 24, 2021 02:53
@ghost ghost added this to the 6.0-rc1 milestone Jul 24, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/3: Cache and reusing streams
5 participants