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

Reuse HttpContext object per HTTP/1 connection and HTTP/2 stream #6424

Merged
merged 3 commits into from Jan 9, 2019

Conversation

@davidfowl
Copy link
Member

commented Jan 5, 2019

  • Today in Kestrel, we reuse the IFeatureCollection per Http1Connection and per Http2Stream. This PR aims to take advantage of that same technique and affinitize the HttpContext and friends so that they are only allocated once per connection.

cc @benaadams

PS: I looked at doing this in the default HttpContextFactory impl with a feature based approach but there's no place to store the HttpContext that isn't per request in other servers.

I'll try to get some perf results for allocations and throughput as well.

Here's a profile before all of the optimizations we've done (including a bunch from @benaadams):

  • Logging is off in this benchmark so there's are no AsyncLocal or ExecutionContext allocations
  • I turned off tiered compilation to reduce the noise (some JIT optimizations are turned off in tier0)

image

I'm still waiting on dotnet/coreclr#21559 to be fixed to get an equivalent profile. Here's what I expect allocation wise:

  • We'll get rid of HttpContext, HttpRequest and HttpResponse allocations as part of this PR
  • The QueueUserWorkItem callbacks should be completely gone by now (latest coreclr (dotnet/coreclr@e7ead79#diff-b9b22fdb097c803c0ca6fa45655b24f7) plus the IThreadPoolWorkItem impl on IOQueue 0d9de49)
  • The byte[] allocations were cut in 1/2 because of this cb1917a and will be fully eliminated once we do the pipelines transition (we'll be using Kestrel buffers instead of allocating temporary ones).
  • The HttpMethod boxing is fixed by this 8be8e00
  • This will also work for HTTP/2 once we implement the pooling as described here #6192

That leaves us with the following allocations:

Here's what I got when looking at allocations with dotTrace's timeline view (thanks @pakrym)

Before

image

After

image

We removed 35MB from the small object heap with this change.

Strings

As for the strings It's all headers and path:

image

@davidfowl davidfowl requested review from halter73 and Tratcher Jan 5, 2019
@davidfowl davidfowl changed the title WIP: Reuse HttpContext object per connection. Reuse HttpContext object per connection. Jan 6, 2019
@davidfowl davidfowl changed the title Reuse HttpContext object per connection. Reuse HttpContext object per HTTP/1 connection and HTTP/2 stream Jan 6, 2019
@@ -535,14 +540,24 @@ private async Task ProcessRequests<TContext>(IHttpApplication<TContext> applicat

InitializeStreams(messageBody);

var httpContext = application.CreateContext(this);
// Initialize the HttpContext before we call into the IHttpApplication
if (_httpContext is null)

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jan 6, 2019

Author Member

If this is a step too far I can revert this commit and go back to this one 8526622. The difference is that it would lazily initialize the http context in the rare case where the IHttpContextFactory is overridden by the user. It means this field would always be null instead of being assigned and those extra calls would be avoided.

@sebastienros

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Before: 1,927,129
After: 1,847,978

4% regression

dotnet run -- --server http://asp-perf-win:5001 --client http://asp-perf-load:5002 `
-j ../Benchmarks/benchmarks.plaintext.json -n Plaintext `
--build-id 67321 --include Microsoft.AspNetCore.Http.* --include Microsoft.AspNetCore.Server.Kestrel.* `
--self-contained -i 5 -x 1
@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

RequestsPerSecond:           1,920,916
Copy link
Member

left a comment

Not a fan of the blanket copy paste, it creates a long term maintenance problem. You can't derive from DefaultHttpContext and then call base, right?

@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

Not a fan of the blanket copy paste, it creates a long term maintenance problem. You can't derive from DefaultHttpContext and then call base, right?

That defeats the purpose. This change effectively makes DefaultHttpContext legacy without breaking existing consumers of it.

- Today in Kestrel, we reuse the IFeatureCollection per connection and per Http2Stream. This PR aims to take advantage of that same technique and affinitize the HttpContext and friends so that they are only allocated per connection.
- ReusableHttpContext and friends mimic the functionality of DefaultHttpContext but is sealed and has no overridable methods.
@davidfowl davidfowl force-pushed the davidfowl/pool-httpcontext branch from de3fa99 to 1623dcf Jan 8, 2019
- Allows servers to cache the HttpContext and friends across requests.
- Remove KestrelHttpContextFactory
@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

@Tratcher 🆙 📅

{
get
{
if (_httpContext is null)

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jan 8, 2019

Author Member

Hmm this change might be a bit dangerous if this is accessed during the request, it'll re-initialize?

This comment has been minimized.

Copy link
@halter73

halter73 Jan 8, 2019

Member

Do you mean if it's accessed outside of the request flow?

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jan 8, 2019

Author Member

Inside. If you get the HttpContext and cast the FeatureCollection property to IHttpContextContainer then it’ll call Initialize again

This comment has been minimized.

Copy link
@benaadams

benaadams Jan 8, 2019

Contributor

Create it upfront in the variable

private ReusableHttpContext _httpContext = new ReusableHttpContext(this);

Add an IsInitialized flag in ReusableHttpContext and use that in the property?

This comment has been minimized.

Copy link
@halter73

halter73 Jan 8, 2019

Member

I'm not sure I understand what you're getting at. You mean something like the following?

app.Run(async context =>
{
    var context2 = ((IHttpContextContainer)context.Features).HttpContext;
    await context.Response.WriteAsync(object.ReferenceEquals(context, context2).ToString());
});

In that case the response would be "True". I'm not sure why you'd think this wouldn't be the case.

I'm also not sure why we're concerned about what happens when the IFeatureCollection to IHttpContextContainer. I don't know why we'd have any burden whatsoever to support that.

This comment has been minimized.

Copy link
@halter73

halter73 Jan 8, 2019

Member

After seeing @benaadams's comment, I understand. Adding a flag that get's reset between requests would certainly fix the issue with ReusableHttpContext.Initialize() getting called multiple times in a request, but I still don't think app code casting the IFeatureCollection to an IHttpContextContainer is a very realistic scenario.

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jan 8, 2019

Author Member

It's super edge case. I'm going to merge this anyways, I just wanted to bring it up.

@davidfowl davidfowl merged commit 688ad19 into master Jan 9, 2019
2 of 3 checks passed
2 of 3 checks passed
AspNetCore-ci
Details
AspNetCore-pr-validation-temp Build #20190108.39 succeeded
Details
license/cla All CLA requirements met.
Details
@davidfowl davidfowl deleted the davidfowl/pool-httpcontext branch Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.