Reduce allocations in HttpRequestUtils.GetUrl()#8203
Merged
andrewlock merged 8 commits intomasterfrom Feb 18, 2026
Merged
Conversation
andrewlock
added
area:tracer
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8203) 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 (8203) - mean (74ms) : 71, 77
master - mean (73ms) : 71, 75
section Bailout
This PR (8203) - mean (78ms) : 76, 79
master - mean (78ms) : 75, 80
section CallTarget+Inlining+NGEN
This PR (8203) - mean (1,066ms) : 1028, 1105
master - mean (1,066ms) : 1014, 1118
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 (8203) - mean (114ms) : 110, 118
master - mean (113ms) : 110, 116
section Bailout
This PR (8203) - mean (116ms) : 114, 119
master - mean (114ms) : 112, 117
section CallTarget+Inlining+NGEN
This PR (8203) - mean (765ms) : 691, 838
master - mean (751ms) : 690, 811
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8203) - mean (103ms) : 100, 106
master - mean (102ms) : 99, 104
section Bailout
This PR (8203) - mean (104ms) : 102, 106
master - mean (102ms) : 100, 104
section CallTarget+Inlining+NGEN
This PR (8203) - mean (755ms) : 693, 817
master - mean (743ms) : 680, 806
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8203) - mean (101ms) : 97, 105
master - mean (99ms) : 96, 102
section Bailout
This PR (8203) - mean (103ms) : 101, 104
master - mean (101ms) : 99, 103
section CallTarget+Inlining+NGEN
This PR (8203) - mean (672ms) : 649, 695
master - mean (661ms) : 641, 681
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 (8203) - mean (192ms) : 188, 196
master - mean (194ms) : 188, 199
section Bailout
This PR (8203) - mean (196ms) : 192, 199
master - mean (196ms) : 193, 199
section CallTarget+Inlining+NGEN
This PR (8203) - mean (1,145ms) : 1075, 1215
master - mean (1,139ms) : 1083, 1194
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 (8203) - mean (276ms) : 271, 281
master - mean (277ms) : 271, 283
section Bailout
This PR (8203) - mean (278ms) : 273, 282
master - mean (276ms) : 272, 280
section CallTarget+Inlining+NGEN
This PR (8203) - mean (930ms) : 883, 976
master - mean (930ms) : 870, 990
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8203) - mean (268ms) : 263, 273
master - mean (270ms) : 264, 276
section Bailout
This PR (8203) - mean (268ms) : 265, 271
master - mean (269ms) : 265, 274
section CallTarget+Inlining+NGEN
This PR (8203) - mean (922ms) : 885, 959
master - mean (920ms) : 887, 953
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8203) - mean (268ms) : 264, 273
master - mean (269ms) : 261, 277
section Bailout
This PR (8203) - mean (269ms) : 264, 273
master - mean (269ms) : 263, 275
section CallTarget+Inlining+NGEN
This PR (8203) - mean (828ms) : 810, 847
master - mean (827ms) : 811, 844
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewlock
force-pushed
the
andrew/http-request-utils
branch
from
2aca1e1 to
cd373af
Compare
BenchmarksBenchmark execution time: 2026-02-18 10:39:14 Comparing candidate commit 3ec7ffa in PR branch Found 2 performance improvements and 10 performance regressions! Performance is the same for 169 metrics, 11 unstable metrics. scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody netcoreapp3.1
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 netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool netcoreapp3.1
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope net472
|
This was referenced Feb 16, 2026
lucaspimentel
approved these changes
Feb 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request optimizes HttpRequestUtils.GetUrl() to reduce memory allocations, particularly for .NET 6+. The optimization introduces conditional compilation to use Uri.GetComponents() on newer runtimes, and includes duck typing to detect the DangerousDisablePathAndQueryCanonicalization flag to avoid incompatibilities.
Changes:
- Added optimized path for .NET 6+ using
Uri.GetComponents()to reduce allocations - Introduced duck typing to detect
DangerousDisablePathAndQueryCanonicalizationflag in Uri - Added comprehensive characterization tests covering various URL formats and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| tracer/src/Datadog.Trace/Util/Http/HttpRequestUtils.cs | Implements performance optimization with conditional compilation for .NET 6+ using GetComponents(), adds duck typing to detect dangerous Uri flags, splits logic into helper methods |
| tracer/test/Datadog.Trace.Tests/Util/Http/HttpRequestUtilsTests.cs | Adds comprehensive test coverage with 183 test cases covering URL formatting, query strings, fragments, encoding, sensitive data redaction, and dangerous Uri creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // With dangerous create. the fragment isn't removed | ||
| [InlineData("http://localhost/path#section", false, true, "http://localhost/path#section")] | ||
| [InlineData("http://localhost/path#section", true, true, "http://localhost/path#section")] | ||
| [InlineData("http://localhost/path?key=value#section", false, true, "http://localhost/path")] |
There was a problem hiding this comment.
The test case on line 50 expects "http://localhost/path" when useDangerousCreate is true and useQueryManager is false, but according to the test on line 48 (without query string), the fragment should be preserved when useDangerousCreate is true. This test case expects the query string AND the fragment to be removed even though dangerous create is enabled. This seems inconsistent with the behavior described in other test cases. Please verify this is the intended behavior and not a typo in the expected value.
Suggested change
| [InlineData("http://localhost/path?key=value#section", false, true, "http://localhost/path")] | |
| [InlineData("http://localhost/path?key=value#section", false, true, "http://localhost/path#section")] |
Member
Author
There was a problem hiding this comment.
I know, it's weird. It's because the section ends up in the querystring in this case 🤷 tl;dr; this is effectively a bug in our code, but it's such an edge case, I don't think it needs prioritizing tbh
andrewlock
added a commit
that referenced
this pull request
Feb 18, 2026
## 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
andrewlock
added a commit
that referenced
this pull request
Feb 18, 2026
## Summary of changes
Reduce allocations for `UriHelpers.CleanUri()` that's called as part of
HTTP requests (among others)
## Reason for change
The allocations showed up in some profiling so decided to dig in. And Oh
Boy, this stuff is _terrible_ on < .NET 6 😅 And it's still terrible
after this PR, just less terrible 😅
## Implementation details
Two main improvements:
- If we need to use a `StringBuilder` to format the `Uri` _anyway_
(because we're stripping identifiers), then use one for the whole
`string`, instead of partially building the string, and then doing
another allocation.
- Use `GetComponents()`, specifying all the components we need, instead
of accessing each of the `Uri` properties ad-hoc. It turns out these
properties are actually very expensive, because they trigger a bunch of
analysis. Doing it one-shot doesn't avoid that, but it still gives some
improvements.
## Test coverage
I wrote a "characterisation" unit test, seeing as we didn't have any, to
describe the current behaviour, to ensure I didn't change anything.
Benchmarking was done (as shown below), comparing the original
implementation with the new implementation, for three different
scenarios:
- `CleanUri`:
```csharp
_uri = new Uri("http://datadoghq.com/some-path");
UriHelpers.CleanUri(_uri, removeScheme: true, tryRemoveIds: true);
```
- `CleanUri_WithIds`:
```csharp
_uriWithIds= new Uri("http://datadoghq.com/some-path/123");
UriHelpers.CleanUri(_uriWithIds, removeScheme: true, tryRemoveIds: true);
```
- `CleanUri_IgnoreIds`:
```csharp
_uriWithIds= new Uri("http://datadoghq.com/some-path/123");
UriHelpers.CleanUri(_uriWithIds, removeScheme: true, tryRemoveIds: false);
```
The benchmark results heavily depend on the `Uri` provided, so it's only
the relative changes that are interesting here, comparing original to
updated.
> Also note that these took a long time to run, I was on a call for some
of it, so the execution times may be a bit wonky, but allocations was
what I was focusing on 😅
<details><summary>Benchmarking code</summary>
<p>
```csharp
[MemoryDiagnoser, GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory), CategoriesColumn]
public class UriHelperBenchmarks
{
private Uri _uri;
private Uri _uriWithIds;
private string _result;
[GlobalSetup]
public void GlobalSetup()
{
_uri = new Uri("http://datadoghq.com/some-path");
_uriWithIds = new Uri("http://datadoghq.com/some-path/123");
}
[GlobalCleanup]
public void GlobalCleanup()
{
}
[Benchmark(Baseline = true)]
[BenchmarkCategory("CleanUri")]
public int CleanUri_Original()
{
_result = OriginalUriHelpers.CleanUri(_uri, removeScheme: true, tryRemoveIds: true);
return _result.Length;
}
[Benchmark(Baseline = true)]
[BenchmarkCategory("CleanUri_WithIds")]
public int CleanUri_WithIds_Original()
{
_result = OriginalUriHelpers.CleanUri(_uriWithIds, removeScheme: true, tryRemoveIds: true);
return _result.Length;
}
[Benchmark(Baseline = true)]
[BenchmarkCategory("CleanUri_IgnoreIds")]
public int CleanUri_IgnoreIds_Original()
{
_result = OriginalUriHelpers.CleanUri(_uriWithIds, removeScheme: true, tryRemoveIds: false);
return _result.Length;
}
[Benchmark]
[BenchmarkCategory("CleanUri")]
public int CleanUri_Updated()
{
_result = UriHelpers.CleanUri(_uri, removeScheme: true, tryRemoveIds: true);
return _result.Length;
}
[Benchmark]
[BenchmarkCategory("CleanUri_WithIds")]
public int CleanUri_WithIds_Updated()
{
_result = UriHelpers.CleanUri(_uriWithIds, removeScheme: true, tryRemoveIds: true);
return _result.Length;
}
[Benchmark]
[BenchmarkCategory("CleanUri_IgnoreIds")]
public int CleanUri_IgnoreIds_Updated()
{
_result = UriHelpers.CleanUri(_uriWithIds, removeScheme: true, tryRemoveIds: false);
return _result.Length;
}
}
```
</p>
</details>
<details><summary>Original benchmarks</summary>
<p>
| Method | Runtime | Mean | Allocated |
| --------------------------- | ------------------ | --------: |
--------: |
| CleanUri_Original | .NET 10.0 | 122.02 ns | 168 B |
| CleanUri_Updated | .NET 10.0 | 92.97 ns | 120 B |
| CleanUri_Original | .NET 6.0 | 162.29 ns | 168 B |
| CleanUri_Updated | .NET 6.0 | 159.69 ns | 120 B |
| CleanUri_Original | .NET Core 2.1 | 263.90 ns | 800 B |
| CleanUri_Updated | .NET Core 2.1 | 237.06 ns | 752 B |
| CleanUri_Original | .NET Core 3.1 | 281.37 ns | 792 B |
| CleanUri_Updated | .NET Core 3.1 | 263.45 ns | 744 B |
| CleanUri_Original | .NET Framework 4.8 | 263.97 ns | 802 B |
| CleanUri_Updated | .NET Framework 4.8 | 216.58 ns | 754 B |
| | | | |
| CleanUri_IgnoreIds_Original | .NET 10.0 | 53.41 ns | 128 B |
| CleanUri_IgnoreIds_Updated | .NET 10.0 | 46.89 ns | 80 B |
| CleanUri_IgnoreIds_Original | .NET 6.0 | 70.11 ns | 128 B |
| CleanUri_IgnoreIds_Updated | .NET 6.0 | 61.18 ns | 80 B |
| CleanUri_IgnoreIds_Original | .NET Core 2.1 | 148.38 ns | 856 B |
| CleanUri_IgnoreIds_Updated | .NET Core 2.1 | 151.01 ns | 800 B |
| CleanUri_IgnoreIds_Original | .NET Core 3.1 | 170.16 ns | 848 B |
| CleanUri_IgnoreIds_Updated | .NET Core 3.1 | 173.91 ns | 800 B |
| CleanUri_IgnoreIds_Original | .NET Framework 4.8 | 159.43 ns | 859 B |
| CleanUri_IgnoreIds_Updated | .NET Framework 4.8 | 145.18 ns | 802 B |
| | | | |
| CleanUri_WithIds_Original | .NET 10.0 | 112.47 ns | 168 B |
| CleanUri_WithIds_Updated | .NET 10.0 | 89.27 ns | 120 B |
| CleanUri_WithIds_Original | .NET 6.0 | 147.53 ns | 168 B |
| CleanUri_WithIds_Updated | .NET 6.0 | 138.39 ns | 120 B |
| CleanUri_WithIds_Original | .NET Core 2.1 | 232.67 ns | 912 B |
| CleanUri_WithIds_Updated | .NET Core 2.1 | 249.10 ns | 856 B |
| CleanUri_WithIds_Original | .NET Core 3.1 | 273.64 ns | 888 B |
| CleanUri_WithIds_Updated | .NET Core 3.1 | 252.48 ns | 840 B |
| CleanUri_WithIds_Original | .NET Framework 4.8 | 246.37 ns | 915 B |
| CleanUri_WithIds_Updated | .NET Framework 4.8 | 248.15 ns | 859 B |
| Method | Runtime | Mean | Allocated |
| --------------------------- | ------------------ | -------: |
--------: |
| CleanUri_Original | .NET 10.0 | 240.0 ns | 472 B |
| CleanUri_Updated | .NET 10.0 | 223.9 ns | 424 B |
| CleanUri_Original | .NET 6.0 | 354.1 ns | 456 B |
| CleanUri_Updated | .NET 6.0 | 351.7 ns | 408 B |
| CleanUri_Original | .NET Core 2.1 | 515.3 ns | 1112 B |
| CleanUri_Updated | .NET Core 2.1 | 498.0 ns | 1064 B |
| CleanUri_Original | .NET Core 3.1 | 547.7 ns | 1096 B |
| CleanUri_Updated | .NET Core 3.1 | 536.6 ns | 1048 B |
| CleanUri_Original | .NET Framework 4.8 | 647.9 ns | 1244 B |
| CleanUri_Updated | .NET Framework 4.8 | 628.0 ns | 1196 B |
| | | | |
| CleanUri_IgnoreIds_Original | .NET 10.0 | 210.6 ns | 440 B |
| CleanUri_IgnoreIds_Updated | .NET 10.0 | 179.5 ns | 272 B |
| CleanUri_IgnoreIds_Original | .NET 6.0 | 317.4 ns | 424 B |
| CleanUri_IgnoreIds_Updated | .NET 6.0 | 293.5 ns | 264 B |
| CleanUri_IgnoreIds_Original | .NET Core 2.1 | 493.0 ns | 1176 B |
| CleanUri_IgnoreIds_Updated | .NET Core 2.1 | 439.8 ns | 1000 B |
| CleanUri_IgnoreIds_Original | .NET Core 3.1 | 505.4 ns | 1160 B |
| CleanUri_IgnoreIds_Updated | .NET Core 3.1 | 463.7 ns | 992 B |
| CleanUri_IgnoreIds_Original | .NET Framework 4.8 | 602.3 ns | 1316 B |
| CleanUri_IgnoreIds_Updated | .NET Framework 4.8 | 559.4 ns | 1139 B |
| | | | |
| CleanUri_WithIds_Original | .NET 10.0 | 247.0 ns | 480 B |
| CleanUri_WithIds_Updated | .NET 10.0 | 252.5 ns | 432 B |
| CleanUri_WithIds_Original | .NET 6.0 | 392.9 ns | 464 B |
| CleanUri_WithIds_Updated | .NET 6.0 | 393.4 ns | 416 B |
| CleanUri_WithIds_Original | .NET Core 2.1 | 598.1 ns | 1232 B |
| CleanUri_WithIds_Updated | .NET Core 2.1 | 579.0 ns | 1176 B |
| CleanUri_WithIds_Original | .NET Core 3.1 | 610.8 ns | 1200 B |
| CleanUri_WithIds_Updated | .NET Core 3.1 | 593.2 ns | 1152 B |
| CleanUri_WithIds_Original | .NET Framework 4.8 | 684.0 ns | 1372 B |
| CleanUri_WithIds_Updated | .NET Framework 4.8 | 688.1 ns | 1316 B |
</p>
</details>
EDIT: I realised that the allocations here aren't quite right, because
this is calling the method on the _same_ `Uri` instance in each
iteration. As `Uri` uses heavy caching, that distorts the allocations a
_lot_. A more accurate test is to use a new `Uri` for each iteration, as
the following benchmarks do. The allocation is more because we're both
allocating the `Uri` _and_ doing the stripping, and the benchmark
doesn't make use of caching, but thankfully we're still better in all
cases.
EDIT 2: Shortly after submitting this PR, I discovered
DangerousDisablePathAndQueryCanonicalization and tl;dr; that's a
nightmare 😅 It's only in .NET 6+, but they don't expose whether it was
set publicly, so had to do some gnarly stuff with duck types. I'm not
proud, but the benchmarks still seem worth pursuing it to me. This is
the same issue that plagues #8203
| Method | Runtime | Mean | Error | Allocated |
| ------------------------------------- | ------------------ |
----------: | ---------: | --------: |
| CleanUri_Original | .NET 10.0 | 241.7598 ns | 3.7925 ns | 472 B |
| CleanUri_Updated | .NET 10.0 | 232.1299 ns | 4.5930 ns | 424 B |
| CleanUri_Original | .NET 6.0 | 360.8269 ns | 6.3565 ns | 456 B |
| CleanUri_Updated | .NET 6.0 | 346.9848 ns | 4.7634 ns | 408 B |
| CleanUri_Original | .NET Core 2.1 | 519.9048 ns | 6.2900 ns | 1112 B |
| CleanUri_Updated | .NET Core 2.1 | 520.4468 ns | 7.2347 ns | 1064 B |
| CleanUri_Original | .NET Core 3.1 | 556.0087 ns | 5.3537 ns | 1096 B |
| CleanUri_Updated | .NET Core 3.1 | 543.6773 ns | 3.7395 ns | 1048 B |
| CleanUri_Original | .NET Framework 4.8 | 632.6677 ns | 5.9946 ns |
1244 B |
| CleanUri_Updated | .NET Framework 4.8 | 632.7827 ns | 12.1315 ns |
1196 B |
| | | | | |
| CleanUri_IgnoreIds_Original | .NET 10.0 | 200.0625 ns | 2.1611 ns |
440 B |
| CleanUri_IgnoreIds_Updated | .NET 10.0 | 172.3196 ns | 3.3675 ns | 272
B |
| CleanUri_IgnoreIds_Original | .NET 6.0 | 337.9443 ns | 6.7787 ns | 424
B |
| CleanUri_IgnoreIds_Updated | .NET 6.0 | 313.6942 ns | 6.1152 ns | 264
B |
| CleanUri_IgnoreIds_Original | .NET Core 2.1 | 493.7106 ns | 9.8265 ns
| 1176 B |
| CleanUri_IgnoreIds_Updated | .NET Core 2.1 | 461.0721 ns | 9.0235 ns |
1000 B |
| CleanUri_IgnoreIds_Original | .NET Core 3.1 | 523.9340 ns | 7.3235 ns
| 1160 B |
| CleanUri_IgnoreIds_Updated | .NET Core 3.1 | 473.7414 ns | 6.9061 ns |
992 B |
| CleanUri_IgnoreIds_Original | .NET Framework 4.8 | 631.2814 ns |
9.9056 ns | 1316 B |
| CleanUri_IgnoreIds_Updated | .NET Framework 4.8 | 605.1636 ns |
10.9916 ns | 1139 B |
| | | | | |
| CleanUri_WithIds_Original | .NET 10.0 | 268.1831 ns | 4.7108 ns | 480
B |
| CleanUri_WithIds_Updated | .NET 10.0 | 268.6544 ns | 8.1581 ns | 432 B
|
| CleanUri_WithIds_Original | .NET 6.0 | 408.5128 ns | 8.4589 ns | 464 B
|
| CleanUri_WithIds_Updated | .NET 6.0 | 381.8052 ns | 4.4549 ns | 416 B
|
| CleanUri_WithIds_Original | .NET Core 2.1 | 572.6368 ns | 5.4855 ns |
1232 B |
| CleanUri_WithIds_Updated | .NET Core 2.1 | 567.9524 ns | 8.8147 ns |
1176 B |
| CleanUri_WithIds_Original | .NET Core 3.1 | 612.2201 ns | 7.8213 ns |
1200 B |
| CleanUri_WithIds_Updated | .NET Core 3.1 | 594.9918 ns | 6.8069 ns |
1152 B |
| CleanUri_WithIds_Original | .NET Framework 4.8 | 700.4916 ns | 13.6586
ns | 1372 B |
| CleanUri_WithIds_Updated | .NET Framework 4.8 | 699.0997 ns | 10.7405
ns | 1316 B |
| | | | | |
| CleanUri_Dangerous_Original | .NET 10.0 | 225.5236 ns | 4.3965 ns |
472 B |
| CleanUri_Dangerous_Updated | .NET 10.0 | 208.1699 ns | 3.5823 ns | 424
B |
| CleanUri_Dangerous_Original | .NET 6.0 | 336.6692 ns | 3.6419 ns | 456
B |
| CleanUri_Dangerous_Updated | .NET 6.0 | 315.4834 ns | 4.6207 ns | 408
B |
| | | | | |
| CleanUri_Dangerous_IgnoreIds_Original | .NET 10.0 | 185.8492 ns |
3.7319 ns | 440 B |
| CleanUri_Dangerous_IgnoreIds_Updated | .NET 10.0 | 190.3548 ns |
3.4563 ns | 440 B |
| CleanUri_Dangerous_IgnoreIds_Original | .NET 6.0 | 277.9524 ns |
4.8131 ns | 424 B |
| CleanUri_Dangerous_IgnoreIds_Updated | .NET 6.0 | 330.9686 ns | 6.3933
ns | 424 B |
| | | | | |
| CleanUri_Dangerous_WithIds_Original | .NET 10.0 | 233.4651 ns | 4.6167
ns | 480 B |
| CleanUri_Dangerous_WithIds_Updated | .NET 10.0 | 225.7008 ns | 4.5418
ns | 432 B |
| CleanUri_Dangerous_WithIds_Original | .NET 6.0 | 355.9743 ns | 6.1388
ns | 464 B |
| CleanUri_Dangerous_WithIds_Updated | .NET 6.0 | 370.6714 ns | 7.1053
ns | 416 B |
## Other details
Spotted while working on
https://datadoghq.atlassian.net/browse/LANGPLAT-842
andrewlock
force-pushed
the
andrew/http-request-utils
branch
from
cd373af to
3ec7ffa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Reduces the allocations that occur in
HttpRequestUtils.GetUrl(), particularly on .NET 6+Reason for change
While profiling a simple aspnetcore + httprequest app (our aspnetcore benchmark), this showed up as a relatively high source of
stringallocations (6%) so wanted to try to reduce that. It was explicitly the version that takes aUrithat showed up, so optimized that somewhat.Implementation details
In newer runtimes (.NET 6+), we can use
GetComponents()to avoid allocating each of the individual pieces of url before formatting. We can also take advantage of the fact this whole path is only valid with absolute Uris, so we know we have aHost, and various other minor things.Otherwise, tried lots of things running various benchmarks 😅
Uriis a tricky beast to deal with, because it does a lot of internal caching too. In general though, we'll be dealing with a newUriinstance every time we call this method, so that's what I focused on. Benchmarked both approaches though, just to be sure, and we come out ahead in both cases.Original benchmarks
EDIT: Shortly after submitting this PR, I discovered
DangerousDisablePathAndQueryCanonicalizationand tl;dr; that's a nightmare 😅 It's only in .NET 6+, but they don't expose whether it was set publicly, so had to do some gnarly stuff with duck types. I'm not proud, but the benchmarks looks worth it to me:Benchmarking code
Test coverage
Before making the change, 🤖 Claude created some characterization tests to avoid any regressions in behaviour.
Other details
There's also
HttpRequestExtensions.GetUrlForSpan()which has similar behaviour and allocations today, that I will likely investigate similarlySpotted while working on https://datadoghq.atlassian.net/browse/LANGPLAT-842