Move vendored ReadOnlySpan<T>, Span<T> et. al. to System namespace#8476
Merged
andrewlock merged 9 commits intomasterfrom Apr 24, 2026
Merged
Move vendored ReadOnlySpan<T>, Span<T> et. al. to System namespace#8476andrewlock merged 9 commits intomasterfrom
ReadOnlySpan<T>, Span<T> et. al. to System namespace#8476andrewlock merged 9 commits intomasterfrom
Conversation
andrewlock
added
type:performance
andrewlock
commented
Apr 17, 2026
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8476) 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 (8476) - mean (75ms) : 71, 79
master - mean (73ms) : 70, 76
section Bailout
This PR (8476) - mean (80ms) : 76, 83
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8476) - mean (1,087ms) : 1015, 1158
master - mean (1,078ms) : 1034, 1122
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 (8476) - mean (117ms) : 109, 124
master - mean (114ms) : 108, 119
section Bailout
This PR (8476) - mean (115ms) : 111, 119
master - mean (114ms) : 110, 117
section CallTarget+Inlining+NGEN
This PR (8476) - mean (779ms) : 754, 803
master - mean (800ms) : 771, 828
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8476) - mean (102ms) : 97, 107
master - mean (104ms) : 99, 109
section Bailout
This PR (8476) - mean (105ms) : 99, 111
master - mean (102ms) : 98, 107
section CallTarget+Inlining+NGEN
This PR (8476) - mean (941ms) : 906, 977
master - mean (942ms) : 896, 988
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8476) - mean (100ms) : 96, 104
master - mean (101ms) : 96, 106
section Bailout
This PR (8476) - mean (104ms) : 97, 111
master - mean (104ms) : 98, 110
section CallTarget+Inlining+NGEN
This PR (8476) - mean (822ms) : 787, 856
master - mean (829ms) : 783, 876
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 (8476) - mean (194ms) : 190, 197
master - mean (193ms) : 189, 196
section Bailout
This PR (8476) - mean (197ms) : 195, 198
master - mean (196ms) : 194, 198
section CallTarget+Inlining+NGEN
This PR (8476) - mean (1,158ms) : 1112, 1203
master - mean (1,158ms) : 1108, 1207
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 (8476) - mean (276ms) : 271, 281
master - mean (276ms) : 271, 281
section Bailout
This PR (8476) - mean (277ms) : 272, 281
master - mean (281ms) : 274, 287
section CallTarget+Inlining+NGEN
This PR (8476) - mean (913ms) : 889, 937
master - mean (946ms) : 921, 971
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8476) - mean (270ms) : 267, 274
master - mean (272ms) : 265, 279
section Bailout
This PR (8476) - mean (270ms) : 267, 274
master - mean (272ms) : 267, 277
section CallTarget+Inlining+NGEN
This PR (8476) - mean (1,119ms) : 1047, 1192
master - mean (1,145ms) : 1104, 1185
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8476) - mean (270ms) : 264, 275
master - mean (273ms) : 263, 283
section Bailout
This PR (8476) - mean (268ms) : 266, 271
master - mean (271ms) : 263, 278
section CallTarget+Inlining+NGEN
This PR (8476) - mean (1,017ms) : 970, 1063
master - mean (1,030ms) : 981, 1079
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-04-23 13:19:48 Comparing candidate commit 0688bae in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 59 known flaky benchmarks, 28 flaky benchmarks without significant changes.
|
This was referenced Apr 20, 2026
andrewlock
force-pushed
the
andrew/update-vendors-7
branch
from
3af08ab to
528b407
Compare
andrewlock
force-pushed
the
andrew/update-vendors-8
branch
from
c448c23 to
ed790e1
Compare
andrewlock
force-pushed
the
andrew/update-vendors-7
branch
from
528b407 to
112589a
Compare
… into System namespace
andrewlock
force-pushed
the
andrew/update-vendors-8
branch
from
ed790e1 to
0688bae
Compare
bouwkast
approved these changes
Apr 23, 2026
| <PackageReference Include="System.Memory" Version="4.5.5" /> | ||
| <PackageReference Include="System.Threading.Tasks.Extensions" Version="4.6.0" /> | ||
| <ProjectReference Include="..\..\..\..\src\Datadog.Trace\Datadog.Trace.csproj" /> | ||
| <ProjectReference Include="..\..\..\..\src\Datadog.Trace\Datadog.Trace.csproj" Aliases="DatadogTrace" /> |
Collaborator
There was a problem hiding this comment.
What is the Aliases="DatadogTrace" for?
Collaborator
There was a problem hiding this comment.
CLAUDE
tracer/build/_build/UpdateVendors/VendoredDependency.cs
Line: 1027
The internalization regex on this line doesn't match public readonly ref partial struct, so the vendored System.Memory refresh leaves the following types public in namespace System inside Datadog.Trace.dll:
tracer/src/Datadog.Trace/Vendors/System.Memory/System/Span.cs:34tracer/src/Datadog.Trace/Vendors/System.Memory/System/Span.Portable.cs:35tracer/src/Datadog.Trace/Vendors/System.Memory/System/ReadOnlySpan.cs:34tracer/src/Datadog.Trace/Vendors/System.Memory/System/ReadOnlySpan.Portable.cs:33
That's what forced the Aliases="DatadogTrace" workaround in CallTargetNativeTest.csproj and the ~30 extern alias DatadogTrace; additions in commit ed790e1838 - Datadog.Trace.dll is now exporting System.Span<T> / System.ReadOnlySpan<T>, which collides with System.Memory 4.5.5 in any consumer that references both without aliases.
Fix: extend this regex to also match partial between ref and struct. For example, change the readonly\s+(ref\s+)?struct alternative to readonly\s+(ref\s+)?(partial\s+)?struct. Then regenerate the vendored files - the four Span types above should become internal. Once they're internal, the Aliases="DatadogTrace" entry in CallTargetNativeTest.csproj and the extern alias directives added in ed790e1838 can be reverted.
Why not just move them out of namespace System: per the comment you added in a18fa35156, the System namespace placement is load-bearing - the C# compiler looks up System.Span<T> by name to light up stackalloc, fixed, slicing, pattern-foreach, etc. Renaming the namespace would break that inside Datadog.Trace.dll on .NET Framework 4.6.1.
Why internal is fine: compiler light-up keys on the type's name and shape, not its accessibility. internal System.Span<T> inside Datadog.Trace.dll produces identical codegen to public System.Span<T> for everything inside that assembly; it just stops leaking the type to downstream consumers - which is the behavior every other vendored type in this repo already has.
Customer impact is low (the manual API ships from Datadog.Trace.Manual.dll, not this assembly), so this doesn't have to block the merge, but worth a follow-up PR before the extern-alias workaround gets copied into more places.****
Member
Author
There was a problem hiding this comment.
Claude is talking nonsense
Member
Author
There was a problem hiding this comment.
We have [InternalsVisibleTo] set to include this project, so the "fix" wouldn't fix anything 😄
Member
Author
There was a problem hiding this comment.
It's right about the root cause though - you get a collision between the System.Span<T> in Datadog.Trace and the System.Span<T> in System.Memory. Unfortunately, this is the only reasonable way to work around the issue. Technically we could remove the System.Memory reference entirely, but I'd rather we kept the "real" references for correctness reasons. Using the extern alias approach means the types aren't automatically available in the namespaces (they aren't in global namespace, they go in the DatadogTrace namespace). I have a post about it here 🙂 https://andrewlock.net/disambiguating-types-with-the-same-name-with-extern-alias/
lucaspimentel
approved these changes
Apr 23, 2026
andrewlock
added a commit
that referenced
this pull request
Apr 24, 2026
## Summary of changes Remove some branching code that's no longer required after #8476 moved `Span<T>` to `System` namespace ## Reason for change This sort of stuff is the _reason_ we made that change, to reduce maintenance. ## Implementation details Set 🤖 looking for possible cases, so it's not exhaustive, but gives a taster. I think most of these make sense. It's nothing outstanding but it's the little things. ## Test coverage Just a refactoring, so covered by existing tests. ## Other details By definition, we don't really expect to see performance improvements for this, other than potentially some reduced allocation in .NET Framework. The primary benefits are devx Depends on the vendoring code stack: Depends on a stack updating our vendored system code - #8391 - #8454 - #8455 - #8459 - #8461 - #8469 - #8476 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
andrewlock
added a commit
that referenced
this pull request
Apr 24, 2026
#8486) ## Summary of changes Update the tag list generator to always use `ReadOnlySpan<byte>` properties instead of `byte[]` ## Reason for change After moving our vendored `Span<T>` implementation into the `System` namespace, various optimizations open up to us, including using `ReadOnlySpan<byte>` properties on .NET Framework instead of `static readonly byte[]` to avoid startup costs. ## Implementation details Replace the code generated by the generator, and update the generated code ## Test coverage Covered by snapshot tests and behaviour is covered by existing tests. We'll check the benchmarks to make sure that we _don't_ see any perf impact (there shouldn't be, impact should just be reduced startup costs) ## Other details Depends on a stack updating our vendored system code - #8391 - #8454 - #8455 - #8459 - #8461 - #8469 - #8476 - #8477
andrewlock
added a commit
that referenced
this pull request
Apr 27, 2026
… string literals (#8487) ## Summary of changes Convert `Encoding.Utf8.GetBytes` calls to static constants using UTF8 string literals ## Reason for change After moving our vendored `Span<T>` implementation into the `System` namespace, various optimizations open up to us, including using UTF-8 string literals, to encode strings to UTF-8 at compile time instead of at runtime. By combing with `static ReadOnlySpan<byte>` properties, these also become zero allocation, so we get reduced overall memory usage as well as better startup time ## Implementation details Replace the following with utf8 string literals: - `StringEncoding.UTF8.GetBytes` - `Encoding.UTF8.GetBytes` - `EncodingHelpers.UTF8NoBom.GetBytes` ## Test coverage All covered by existing tests ## Other details Depends on a stack updating our vendored system code - #8391 - #8454 - #8455 - #8459 - #8461 - #8469 - #8476 - #8477 - #8486
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
Updates the System.Memory vendoring code to move types like
ReadOnlySpan<T>,Span<T>intoSysteminstead ofDatadog.Trace.VendoredMicrosoftCode.SystemReason for change
The compiler has various functionality that relies on the
Span<T>(andReadOnlySpan<T>) being available in theSystemnamespace. By making this change, we get the advantage of those types being available.A separate PR will actually update code to use those types, except where changes were required to make it compile in this PR.
Implementation details
Update the vendoring code to stop changing the namespace.
Test coverage
This is the test, if the tests pass, we should be fine.
Other details
Depends on a stack updating our vendored system code