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

Remove some GCHandles from SendHeaders() #44561

Merged
merged 25 commits into from Oct 24, 2022

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Oct 14, 2022

Description

Fixes #40215

This introduces a new type, UnmanagedBufferAllocator, that manages unmanaged block allocations. The intent here is to remove the allocation of handles and temporary managed arrays and replace them with native memory.

Instead of using an unmanaged memory allocator like NativeMemory, the GC.AllocateUninitializedArray() could be used to create pinned memory. I've rejected this because it puts the onus on the GC to collect the memory whereas with the unmanaged memory it can be freed precisely when it is no longer needed. Since the pinned heap is presently considered only on a gen2 collection it leaves this unused memory around far longer than needed and creates unnecessary pressure.

This introduces a new IBufferWriter implementation
that manages unmanaged block allocations. The
intent here was to remove the allocation of handles
amd temporary managed arrays allocations and
and replace them with native memory.
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Remove some GCHandles from SendHeaders() Remove some GCHandles from SendHeaders() Oct 14, 2022
@AaronRobinsonMSFT
Copy link
Member Author

/benchmark list

@pr-benchmarks
Copy link

pr-benchmarks bot commented Oct 15, 2022

Crank Pull Request Bot

/benchmark <benchmarks[,...]> <profiles[,...]> <components,[...]>

Benchmarks:

  • plaintext: TechEmpower Plaintext Scenario - ASP.NET Platform implementation
  • json: TechEmpower JSON Scenario - ASP.NET Platform implementation
  • fortunes: TechEmpower Fortunes Scenario - ASP.NET Platform implementation
  • yarp: YARP - http-http with 10 bytes
  • mvcjsoninput2k: Sends 2Kb Json Body to an MVC controller

Profiles:

  • aspnet-perf-lin: INTEL/Linux 12 Cores
  • aspnet-perf-win: INTEL/Windows 12 Cores
  • aspnet-citrine-lin: INTEL/Linux 28 Cores
  • aspnet-citrine-win: INTEL/Windows 28 Cores
  • aspnet-citrine-arm: ARM/Linux 32 Cores
  • aspnet-citrine-amd: AMD/Linux 48 Cores

Components:

  • kestrel
  • mvc

@davidfowl
Copy link
Member

Http.sys isn't in this list. I recently did a perf change to http.sys and had to build locally and run a benchmark. We should add the JSON one.

cc @sebastienros @adityamandaleeka @Tratcher

an empty Span<>.
Zero initialize count member.
Remove use of IBufferWriter<T> and replace with
struct with methods for scenarios.
Move encoding logic to use Encoding.UTF8 directly.
Plumb UnmanagedBufferAllocator through APIs.
Remove many GCHandle allocations.
Add UnmanagedBufferAllocator field to ResponseStreamAsyncResult.
always allocate precisely what is needed.

Fix an unsafe exception code path.
src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/Response.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/Response.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/Response.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs Outdated Show resolved Hide resolved
src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs Outdated Show resolved Hide resolved
Remove explicit type static constructor.
@AaronRobinsonMSFT

This comment was marked as outdated.

@AaronRobinsonMSFT

This comment was marked as outdated.

@jkotas
Copy link
Member

jkotas commented Oct 18, 2022

LGTM!

@AaronRobinsonMSFT
Copy link
Member Author

@davidfowl @Tratcher @JamesNK Please take another look. The CI is all green.

@davidfowl
Copy link
Member

So, it's a bit slower or a wash and the GC numbers look about the same. 🤔

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Oct 20, 2022

Sharing offline comments I made based on collection of a nettrace for GCHandle usage.

I'm convinced on this change. The logging itself is a win. The number of GCHandle events is staggering. I did a quick experiment locally. Instead of using NativeMemory.Alloc and NativeMemory.Free, I replaced them with ArrayPool<byte>.Shared.Rent and a pinned GCHandle. The trace resulted in precisely the same experience before my larger changes. This is a win and the performance characteristics that we've improved aren't reflected in the crank base statistics. They are however reflected in nettrace.

The nettrace impact is prior to this change the nettrace was 1GB, but after this change it is approximately 40MB. The win here is reduction of the impact of GCHandle events.

@davidfowl
Copy link
Member

@sebastienros I want to assign this to you. Once we have benchmarks running on .NET 8 we should merge this.

@sebastienros
Copy link
Member

@davidfowl we are tracking net8.0 now (check the charts) so I assume that's what you wanted before we merge the PR.

@davidfowl
Copy link
Member

Yep

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit a5ae53a into dotnet:main Oct 24, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the remove_gchandles branch October 24, 2022 05:48
@ghost ghost added this to the 8.0-preview1 milestone Oct 24, 2022
@BrennanConroy
Copy link
Member

The nettrace impact is prior to this change the nettrace was 1GB, but after this change it is approximately 40MB. The win here is reduction of the impact of GCHandle events.

Could you share what nettrace args are needed to show this? I'm thinking ahead to next year when we write a ASP.NET Core 8 perf improvements blog post and it's useful to have info on the PR so we don't need to figure out how to repro things a year later.

@AaronRobinsonMSFT
Copy link
Member Author

@BrennanConroy This is easy to see by adding the following arguments to the crank command: --application.dotnetTrace true --application.dotnetTraceProviders gc-verbose. Running scenarios where a lot of GCHandles instance are created/destroyed will saturate the pipeline and result in dropped events or at best, very large nettrace files.

@ghost

This comment was marked as spam.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-httpsys Perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Http.Sys response header serialization allocations
7 participants