Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Kestrel should implement IHttpRequestIdentifierFeature to avoid the slow default implementation #235

Closed
AspNetSmurfLab opened this issue Oct 1, 2015 · 12 comments

Comments

@AspNetSmurfLab
Copy link

Perhaps use the socket handle ID combined with a simple counter of some sort. Make it as "free" as possible 😄

by @DamianEdwards

@Tratcher
Copy link
Member

Tratcher commented Oct 1, 2015

We also need this in order to take full advantage of the static feature collection from #162

@davidfowl
Copy link
Member

I actually think we should make the default implementation not slow 😄

@AspNetSmurfLab
Copy link
Author

FYI, CPU profile of latest Kestrel showing that this is currently the most expensive item in the plaintext benchmark, accounting for 7.17% of all CPU time.

by @DamianEdwards

@muratg muratg added this to the 1.0.0-rc1 milestone Oct 2, 2015
@AspNetSmurfLab
Copy link
Author

Trying to make the default implementation in Hosting faster first: https://github.com/aspnet/Hosting/tree/damianedwards/fast-request-id

by @DamianEdwards

@muratg
Copy link
Contributor

muratg commented Oct 8, 2015

@DamianEdwards Link to your branch on hosting doesn't work. Is that merged or deleted?

@davidfowl
Copy link
Member

Hosting has a very fast implementation that is already merged now.

@muratg
Copy link
Contributor

muratg commented Oct 8, 2015

So this one good to close, or do we plan to do more on Kestrel to gain more perf?

@muratg muratg modified the milestones: 1.0.0 Backlog, 1.0.0-rc1 Oct 8, 2015
@Tratcher
Copy link
Member

Tratcher commented Oct 8, 2015

Kestrel could still do better than Hosting, such as #243

@benaadams
Copy link
Contributor

Or can push up to HttpAbstractions and add TraceIdentifier onto HttpContext which then makes it available for wider use aspnet/HttpAbstractions#435

Then switch to lazy eval? aspnet/Hosting#394, no change in Kestrel, all aspnet servers go faster.

@muratg muratg modified the milestones: Backlog, 1.0.0 backlog Dec 11, 2015
@muratg
Copy link
Contributor

muratg commented Dec 11, 2015

Not sure if this is still relevant. @DamianEdwards feel free to close.

@sajayantony
Copy link
Member

@halter73 wanted to get pulse on this issue. Is it still relevant or did you break it into some other issue.

@halter73
Copy link
Member

I'm closing this issue. Feel free to reopen, but since the time this issue was filed, the default IHttpRequestIdentifierFeature was changed to both lazily evaluate the request identifier and cache the result. I don't think there's anything about generating the identifier in Kestrel itself that could really make this faster.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants