-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Reduce HTTP/2 allocations #6119
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
Conversation
adc4e41
to
abf8dea
Compare
Metrics? |
Less allocations 😬. Honestly though I don’t have a good way to build this locally. I’ll try again but this change is better generally. Even if throughout doesn’t change. |
I’ll try running something. |
Also dotMemory is broken with the latest .NET Core ci builds https://github.com/dotnet/coreclr/issues/21559 |
Gimme a sign off bros! I tried to use Perfview but I can’t read allocations in that thing |
You should add that you are in a place where internet is limited and downloading the artifacts from the CI under VPN is slow. I will run the benchmarks for you then. I have access to the CI. |
If running these benchmarks works. I'd love to show before and after allocations as well but that can wait I guess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of makes you wish we never made ProcessRequestsAsync a generic method and just went the generic type route to begin with.
Thanks for the conflicts @halter73 |
- Remove per request allocations on the thread pool by implementing IThreadPoolWorkItem on Http2Stream - Made generic version of Http2Stream to store the IHttpApplication instead of using a tuple - Removed passing of IHttpApplication<TContext> everywhere
abf8dea
to
40a2d4a
Compare
If anyone has advice on how to best analyze allocations using perfview, I'm interested. I got the following using the "DotNetAlloc" flag. BeforeAfterIt shows the JIT_ClassInitDymamicClass and JIT_New events are now longer emitted during StartStream() which is a good sign. There's still a JIT_NewArray1 remaining. I loaded symbols after taking the screenshots to confirm this is from resizing the _streams Dictionary. I don't really know what the Inc/Exc %/Ct columns mean in the context of the "GC Heap Net Mem Stacks" view. |
cc @benaadams
PS: I can't run much locally, so the CI will tell me if it doesn't work 😄