Improve performance of AspNetCoreResourceNameHelper.SimplifyRouteTemplate#8170
Improve performance of AspNetCoreResourceNameHelper.SimplifyRouteTemplate#8170andrewlock merged 8 commits intomasterfrom
AspNetCoreResourceNameHelper.SimplifyRouteTemplate#8170Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93f80eede0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs
Outdated
Show resolved
Hide resolved
lucaspimentel
left a comment
There was a problem hiding this comment.
LGTM aside from the build errors:
tracer\src\Datadog.Trace\DiagnosticListeners\AspNetCoreResourceNameHelper.cs(262,28):
error CS0246: The type or namespace name 'ValueStringBuilder' could not be found (are you missing a using directive or an assembly reference?)
93f80ee to
c886897
Compare
Oops, messed up my rebasing! 🤦♂️ Fixed now. Also added some extra unit tests for |
BenchmarksBenchmark execution time: 2026-02-18 10:25:15 Comparing candidate commit bca39bf in PR branch Found 8 performance improvements and 3 performance regressions! Performance is the same for 171 metrics, 10 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest net6.0
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest netcoreapp3.1
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync net472
scenario:Benchmarks.Trace.Log4netBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan netcoreapp3.1
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8170) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (73ms) : 71, 75
master - mean (74ms) : 72, 76
section Bailout
This PR (8170) - mean (77ms) : 76, 79
master - mean (79ms) : 76, 82
section CallTarget+Inlining+NGEN
This PR (8170) - mean (1,064ms) : 1018, 1110
master - mean (1,077ms) : 1036, 1119
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (114ms) : 110, 117
master - mean (116ms) : 113, 120
section Bailout
This PR (8170) - mean (114ms) : 112, 115
master - mean (117ms) : 114, 120
section CallTarget+Inlining+NGEN
This PR (8170) - mean (755ms) : 693, 817
master - mean (766ms) : 704, 827
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (102ms) : 98, 105
master - mean (103ms) : 100, 106
section Bailout
This PR (8170) - mean (103ms) : 101, 104
master - mean (104ms) : 102, 107
section CallTarget+Inlining+NGEN
This PR (8170) - mean (744ms) : 684, 805
master - mean (759ms) : 697, 822
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (100ms) : 97, 103
master - mean (102ms) : 99, 104
section Bailout
This PR (8170) - mean (102ms) : 99, 104
master - mean (103ms) : 100, 105
section CallTarget+Inlining+NGEN
This PR (8170) - mean (669ms) : 651, 687
master - mean (677ms) : 648, 706
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (192ms) : 189, 196
master - mean (194ms) : 190, 199
section Bailout
This PR (8170) - mean (196ms) : 193, 199
master - mean (198ms) : 195, 200
section CallTarget+Inlining+NGEN
This PR (8170) - mean (1,141ms) : 1083, 1198
master - mean (1,144ms) : 1081, 1208
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (277ms) : 272, 283
master - mean (278ms) : 272, 284
section Bailout
This PR (8170) - mean (277ms) : 274, 280
master - mean (278ms) : 275, 282
section CallTarget+Inlining+NGEN
This PR (8170) - mean (929ms) : 884, 974
master - mean (932ms) : 879, 984
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (270ms) : 264, 277
master - mean (271ms) : 266, 276
section Bailout
This PR (8170) - mean (270ms) : 265, 275
master - mean (270ms) : 266, 275
section CallTarget+Inlining+NGEN
This PR (8170) - mean (923ms) : 882, 963
master - mean (930ms) : 890, 970
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8170) - mean (268ms) : 262, 273
master - mean (273ms) : 264, 282
section Bailout
This PR (8170) - mean (267ms) : 265, 270
master - mean (273ms) : 267, 279
section CallTarget+Inlining+NGEN
This PR (8170) - mean (832ms) : 808, 855
master - mean (831ms) : 814, 848
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
## Summary of changes Makes `ValueStringBuilder` usable in .NET Core 3.1+, instead of just .NET 6 ## Reason for change I want to use it in some aspnetcore improvements, but currently it's only exposed in .NET 6 ## Implementation details Update the `#if`. Note that I originally made this available in _all_ TFMs, using our vendored spans, but benchmarking showed that this was actively harmful compared to just using `StringBuilderCache`, so to avoid incorrect use, just restricting it for now. ## Test coverage Updated the tests to cover .NET Core 3.1 too. ## Other details Part of a stack https://datadoghq.atlassian.net/browse/LANGPLAT-842 - #8167 👈 - #8170
4172b5b to
6cc2185
Compare
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the performance of AspNetCoreResourceNameHelper.SimplifyRouteTemplate() to reduce memory allocations and improve execution speed for ASP.NET Core route template processing. The method is used primarily in pre-endpoint routing flows to generate resource names from route templates.
Changes:
- Introduced
ValueStringBuilderfor stack-allocated string building on .NET Core 3.1+ to reduce heap allocations - Optimized
IsIdentifierSegment()to fast-track common numeric and GUID types, avoiding unnecessaryToString()calls - Removed enumerator boxing by explicitly casting
routePattern.SegmentstoList<TemplateSegment>
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs | Refactored SimplifyRouteTemplate() to use ValueStringBuilder on .NET Core 3.1+ and optimized IsIdentifierSegment() with type-based fast paths |
| tracer/test/Datadog.Trace.Tests/DiagnosticListeners/AspNetCoreResourceNameHelperTests.cs | Added comprehensive test coverage for IsIdentifierSegment() behavior with various input types |
| tracer/test/Datadog.Trace.Tests/Util/UriHelpersTests.cs | Added extensive test cases for UriHelpers.IsIdentifierSegment() covering edge cases like GUIDs, hex strings, and numeric patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs
Show resolved
Hide resolved
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs
Show resolved
Hide resolved
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs
Show resolved
Hide resolved
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs
Show resolved
Hide resolved
## Summary of changes A few minor improvements to the standard `AspNetCoreDiagnosticObserver` ## Reason for change Looking into obvious perf improvements for ASP.NET Core, but only a few minor things stood out (apart from related PRs like #8199 and #8203). ## Implementation details - Reduce size of MVC tags object by not deriving from `WebTags` (we never set those tags anyway) - Delay creating spanlinks collection if we don't need it - HttpRoute always matches AspNetCoreRoute, so can make it readonly ## Test coverage Covered by existing tests, benchmarks show (tiny) allocation gains ## Other details Relates to https://datadoghq.atlassian.net/browse/LANGPLAT-842 Related PRs: - #8167 - #8170 - #8180 - #8196 - #8199 - #8203
Add tests for both implementations, to document existing behaviour
6cc2185 to
b02265c
Compare
…tern` (#8180) ## Summary of changes Switches to using `ValueStringBuilder` and other minor perf improvements ## Reason for change Same as #8170, we want to improve performance where we can. Unfortunately, we can't easily avoid the enumeration allocation like we did in that PR, so most of the benefits here are simply from using `ValueStringBuilder` and other minor changes. ## Implementation details Incorporated various changes based on the `SimplifyRoutePattern` that we use in the single-span aspnetcore observer and the changes made in #8170. The gains aren't as high here, because we can't reduce enumeration allocation. ## Test coverage Covered by existing tests. Ran benchmarks using the values. In general, there's a slight regression in duration for an improvement in Alloc Ratio. It's not very consistent though, particularly in <.NET Core 3.1. I think it's probably still worth the change, but open to thoughts | Method | Runtime | Has Defaults | Expand Templates | Mean | Allocated | Alloc Ratio | | -------- | ------------- | ------------ | ---------------- | --------: | --------: | ----------: | | Original | .NET 10.0 | False | False | 6.605 us | 6.51 KB | 1.00 | | Updated | .NET 10.0 | False | False | 8.559 us | 4.8 KB | 0.74 | | Original | .NET 6.0 | False | False | 11.466 us | 6.51 KB | 1.00 | | Updated | .NET 6.0 | False | False | 11.654 us | 4.8 KB | 0.74 | | Original | .NET Core 3.1 | False | False | 13.808 us | 6.51 KB | 1.00 | | Updated | .NET Core 3.1 | False | False | 15.266 us | 4.8 KB | 0.74 | | Original | .NET Core 3.0 | False | False | 13.962 us | 6.51 KB | 1.00 | | Updated | .NET Core 3.0 | False | False | 19.757 us | 6.51 KB | 1.00 | | | | | | | | | | Original | .NET 10.0 | False | True | 6.453 us | 6.2 KB | 0.99 | | Updated | .NET 10.0 | False | True | 8.116 us | 4.65 KB | 0.75 | | Original | .NET 6.0 | False | True | 11.121 us | 6.23 KB | 1.00 | | Updated | .NET 6.0 | False | True | 11.375 us | 4.65 KB | 0.75 | | Original | .NET Core 3.1 | False | True | 14.075 us | 6.23 KB | 1.00 | | Updated | .NET Core 3.1 | False | True | 15.000 us | 4.65 KB | 0.75 | | Original | .NET Core 3.0 | False | True | 14.027 us | 6.23 KB | 1.00 | | Updated | .NET Core 3.0 | False | True | 13.687 us | 6.2 KB | 0.99 | | | | | | | | | | Original | .NET 10.0 | True | False | 6.348 us | 6.51 KB | 1.00 | | Updated | .NET 10.0 | True | False | 8.310 us | 4.8 KB | 0.74 | | Original | .NET 6.0 | True | False | 11.082 us | 6.51 KB | 1.00 | | Updated | .NET 6.0 | True | False | 11.406 us | 4.8 KB | 0.74 | | Original | .NET Core 3.1 | True | False | 9.853 us | 6.51 KB | 1.00 | | Updated | .NET Core 3.1 | True | False | 10.865 us | 4.8 KB | 0.74 | | Original | .NET Core 3.0 | True | False | 14.879 us | 6.51 KB | 1.00 | | Updated | .NET Core 3.0 | True | False | 10.221 us | 6.51 KB | 1.00 | | | | | | | | | | Original | .NET 10.0 | True | True | 4.002 us | 6.2 KB | 0.99 | | Updated | .NET 10.0 | True | True | 5.685 us | 4.65 KB | 0.75 | | Original | .NET 6.0 | True | True | 7.964 us | 6.23 KB | 1.00 | | Updated | .NET 6.0 | True | True | 8.385 us | 4.65 KB | 0.75 | | Original | .NET Core 3.1 | True | True | 13.949 us | 6.23 KB | 1.00 | | Updated | .NET Core 3.1 | True | True | 15.370 us | 4.65 KB | 0.75 | | Original | .NET Core 3.0 | True | True | 10.116 us | 6.23 KB | 1.00 | | Updated | .NET Core 3.0 | True | True | 10.241 us | 6.2 KB | 0.99 | ## Other details Part of a stack https://datadoghq.atlassian.net/browse/LANGPLAT-842 - #8167 - #8170 - #8180 👈
Summary of changes
Reduces the allocations produced in
AspNetCoreResourceNameHelper.SimplifyRouteTemplate()Reason for change
I'm working on perf for aspnetcore in general, and this was an easy target point, as we know it allocates a bunch of intermediate
stringcurrently.Note that I believe this is generally only used in the pre-endpoint routing flows, so it will not be commonly used. It doesn't hurt though, and it was easier to work with, hence it's first up here 😄
Implementation details
This is mostly inspired/based on the work I did in the single-span optimized path, but accounts for the fact that
controller,action, andareafor tags, so those params are already knownThe high-level improvements are:
ValueStringBuilderfor building up the span (.NET Core 3.1+). The main benefit that gives is theAppendAsLowerInvariantmethodList<T>(it's always that type in all implementations, so this is an easy improvement)controller,action, andareavaluesIsIdentifierSegment()to check for common identifier types that are always going to be ommited. Removes the need for callingToString()onintvalues (for example).NET Core 2.1/3.0 (where we can't use
ValueStringBuilder) has the following differencesStringBuilderCacheinstead ofValueStringBuilderAppendAsLowerInvariant()and callToString().ToLowerInvariant()at the end (as we do today)Test coverage
We already have unit tests for the behaviour, so I focused on the benchmarks
For the first test, I tested against all the templates we test with here:
dd-trace-dotnet/tracer/test/Datadog.Trace.Tests/DiagnosticListeners/AspNetCoreResourceNameHelperTests.cs
Lines 104 to 122 in c0dad86
and created "aggregated" results, testing against .NET Core 2.1, 3.1, 6, and 10:
In all cases, we allocate less now, and for all .NET Core 3.1+, a lot less. There is one minor regression in .NET Core 3.1 in speed when we expand params, but I think the allocation savings are still worth it.
To ensure there were no single degenerate cases, I also ran .NET 6 only against each template independently.
Full single-template benchmark results
In almost all cases there were improvements in both memory and cpu (see details collapsed above), but there were a few cases that stood out as getting worse. The problematic templates (and their results) were:
The first case is the most odd. I can't for the life of me see where the extra allocation is coming from 😕 The second case is similarly strange that there's no reduction in allocation. The latter one shows a degradation in time only, so is less concerning, but still strange! 😄
I am a bit concerned about the first case, but even putting dotmemmory on it, the only allocations I see are the final
ToString()so 🤷♂️Benchmark code
Other details
Part of a stack
https://datadoghq.atlassian.net/browse/LANGPLAT-842
ValueStringBuilderavailable in.NET Core 3.1#8167AspNetCoreResourceNameHelper.SimplifyRouteTemplate#8170 👈AspNetCoreResourceNameHelper.SimplifyRoutePattern#8180