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

Reduce heap allocations in AspNetCoreDiagnosticObserver #779

Merged
merged 4 commits into from
Jul 8, 2020

Conversation

kevingosse
Copy link
Collaborator

  • Use a wrapper on top of IHeaderDictionary instead of duplicating the headers
  • Make all methods that expect a IHeadersCollection generic, to avoid boxing the wrapper
  • Cache the SamplingPriority values in a dictionary to avoid the costly enum conversion
  • Remove the ToList when accessing the header values

Benchmark:

AspNetCoreDiagnosticObserver.ExtractPropagatedContext(request)
  • No headers: request with empty headers
  • All headers: request with headers TraceId / ParentId / SamplingPriority
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.19041
Intel Core i7-9750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=3.1.301
  [Host]     : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
  DefaultJob : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
NoHeaders_Old 95.84 ns 1.299 ns 1.215 ns 0.0305 - - 192 B
NoHeaders_New 32.65 ns 0.177 ns 0.147 ns 0.0038 - - 24 B
AllHeaders_Old 998.29 ns 21.315 ns 19.938 ns 0.1640 - - 1032 B
AllHeaders_New 226.51 ns 5.037 ns 6.186 ns 0.0443 - - 280 B

@kevingosse kevingosse requested a review from a team as a code owner July 2, 2020 18:13
{
if (_headers.TryGetValue(name, out var values))
{
return values.ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

This ToArray() allocates another string array and copy the content (https://source.dot.net/#System.Linq/EnumerableHelpers.Linq.cs,105) maybe we can avoid it by using:

Suggested change
return values.ToArray();
return values as string[] ?? values.ToArray();

what do you think?

Copy link
Collaborator Author

@kevingosse kevingosse Jul 3, 2020

Choose a reason for hiding this comment

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

This is StringValues.ToArray(), not Enumerable.ToArray() (confusing, I know)
https://source.dot.net/#Microsoft.Extensions.Primitives/StringValues.cs,280

Copy link
Member

Choose a reason for hiding this comment

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

ok, yeah didn't realize that, this struct is only used by the HttpRequest.Headers

Copy link
Member

@lucaspimentel lucaspimentel left a comment

Choose a reason for hiding this comment

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

I like this much better, thanks!

Copy link
Collaborator

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

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

Nice!

- Use a wrapper on top of IHeaderDictionary instead of duplicating the headers
- Make all methods that expect a IHeadersCollection generic, to avoid boxing the wrapper
- Cache the SamplingPriority values in a dictionary to avoid the costly enum conversion
- Remove the ToList when accessing the header values
@kevingosse kevingosse force-pushed the kevin/aspnetcorediagnosticobserver branch from 16814ae to bb5b68e Compare July 8, 2020 11:20
@kevingosse kevingosse merged commit 6d72a76 into master Jul 8, 2020
@kevingosse kevingosse deleted the kevin/aspnetcorediagnosticobserver branch July 8, 2020 12:26
@zacharycmontoya zacharycmontoya added this to the 1.18.2 milestone Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants