Update vendored System.Collections.Immutable#8391
Merged
Merged
Conversation
andrewlock
force-pushed
the
andrew/update-vendors
branch
from
April 1, 2026 11:27
ffd7780 to
ab9dd65
Compare
andrewlock
commented
Apr 1, 2026
Comment on lines
+412
to
+413
| // Hacky, but it works | ||
| contents = contents.Replace("var sortedDictionary = this.Dictionary as ImmutableSortedDictionary<TKey, TValue>;", "var sortedDictionary = this.Dictionary as IImmutableDictionaryInternal<TKey, TValue>;"); |
Member
Author
There was a problem hiding this comment.
This is the hackiest of the fixes, but it's quite useful. Here is the impact in context:
public override bool Contains(TValue item)
{
- var sortedDictionary = this.Dictionary as ImmutableSortedDictionary<TKey, TValue>;
+ var sortedDictionary = this.Dictionary as IImmutableDictionaryInternal<TKey, TValue>;
if (sortedDictionary != null)
{
return sortedDictionary.ContainsValue(item);
}
var dictionary = this.Dictionary as IImmutableDictionaryInternal<TKey, TValue>;
if (dictionary != null)
{
return dictionary.ContainsValue(item);
}
throw new NotSupportedException();
}this is the only reference to ImmutableSortedDictionary<> we have, so it could never be true anyway AFAICT, and it means that we can completely remove the ImmutableSortedDictionary<> hierarchy, which trims a bunch of other stuff too
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffd778022c
ℹ️ 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".
andrewlock
commented
Apr 1, 2026
| return Regex.Replace( | ||
| result, | ||
| @"public(\s+((abstract|sealed|static|unsafe)\s+)*?(partial\s+)?(class|readonly\s+(ref\s+)?struct|struct|interface|enum|delegate))", | ||
| @"public(\s+((abstract|sealed|static|unsafe)\s+)*?(readonly\s+)?(partial\s+)?(class|readonly\s+(ref\s+)?struct|struct|interface|enum|delegate))", |
Member
Author
There was a problem hiding this comment.
Necessary because there was one instance of public readonly partial class, which we didn't support previously
andrewlock
force-pushed
the
andrew/update-vendors
branch
from
April 1, 2026 11:39
ab9dd65 to
445b715
Compare
andrewlock
marked this pull request as draft
April 1, 2026 13:38
andrewlock
force-pushed
the
andrew/update-vendors
branch
2 times, most recently
from
April 1, 2026 14:17
43cbd2b to
3c2c93a
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8391) 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 (8391) - mean (73ms) : 69, 77
master - mean (73ms) : 70, 77
section Bailout
This PR (8391) - mean (79ms) : 75, 84
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8391) - mean (1,141ms) : 964, 1317
master - mean (1,075ms) : 1030, 1121
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 (8391) - mean (120ms) : 113, 127
master - mean (116ms) : 108, 123
section Bailout
This PR (8391) - mean (118ms) : 114, 122
master - mean (115ms) : 111, 118
section CallTarget+Inlining+NGEN
This PR (8391) - mean (801ms) : 772, 831
master - mean (797ms) : 773, 821
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8391) - mean (101ms) : 97, 104
master - mean (102ms) : 95, 108
section Bailout
This PR (8391) - mean (102ms) : 99, 105
master - mean (103ms) : 98, 108
section CallTarget+Inlining+NGEN
This PR (8391) - mean (945ms) : 911, 979
master - mean (940ms) : 906, 974
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8391) - mean (101ms) : 96, 106
master - mean (100ms) : 96, 103
section Bailout
This PR (8391) - mean (106ms) : 100, 112
master - mean (103ms) : 97, 108
section CallTarget+Inlining+NGEN
This PR (8391) - mean (828ms) : 785, 870
master - mean (825ms) : 786, 865
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 (8391) - mean (192ms) : 188, 196
master - mean (191ms) : 188, 195
section Bailout
This PR (8391) - mean (195ms) : 193, 197
master - mean (195ms) : 193, 196
section CallTarget+Inlining+NGEN
This PR (8391) - mean (1,152ms) : 1103, 1202
master - mean (1,153ms) : 1101, 1205
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 (8391) - mean (275ms) : 269, 280
master - mean (274ms) : 270, 278
section Bailout
This PR (8391) - mean (275ms) : 272, 279
master - mean (275ms) : 271, 279
section CallTarget+Inlining+NGEN
This PR (8391) - mean (941ms) : 917, 966
master - mean (930ms) : 903, 957
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8391) - mean (269ms) : 264, 274
master - mean (267ms) : 263, 272
section Bailout
This PR (8391) - mean (269ms) : 266, 272
master - mean (268ms) : 265, 271
section CallTarget+Inlining+NGEN
This PR (8391) - mean (1,142ms) : 1109, 1174
master - mean (1,138ms) : 1099, 1176
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8391) - mean (266ms) : 262, 271
master - mean (266ms) : 261, 271
section Bailout
This PR (8391) - mean (267ms) : 264, 270
master - mean (266ms) : 263, 268
section CallTarget+Inlining+NGEN
This PR (8391) - mean (1,023ms) : 985, 1061
master - mean (1,019ms) : 981, 1057
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewlock
marked this pull request as ready for review
April 1, 2026 15:09
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c2c93a94a
ℹ️ 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".
andrewlock
force-pushed
the
andrew/update-vendors
branch
from
April 2, 2026 08:33
db7cc20 to
23014ee
Compare
andrewlock
force-pushed
the
andrew/update-vendors
branch
from
April 14, 2026 11:21
23014ee to
9f49119
Compare
This was referenced Apr 14, 2026
BenchmarksBenchmark execution time: 2026-04-21 11:24:03 Comparing candidate commit ffcd2fb in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics, 87 known flaky benchmarks.
|
This was referenced Apr 15, 2026
andrewlock
force-pushed
the
andrew/update-vendors
branch
from
April 16, 2026 13:16
9f49119 to
7ea4e03
Compare
This was referenced Apr 17, 2026
…ally need (by commenting out a type check)
andrewlock
force-pushed
the
andrew/update-vendors
branch
from
April 21, 2026 10:18
7ea4e03 to
ffcd2fb
Compare
NachoEchevarria
approved these changes
Apr 22, 2026
andrewlock
added a commit
that referenced
this pull request
Apr 22, 2026
## Summary of changes Use the built-in _System.Reflection.Metadata_ types for .NET Core 3.1+ ## Reason for change In #6726, we switched from always using the vendored System.Memory and other BCL types, to using the built-in versions for .NET Core 3.1 where they're available. This was done with the goal of reducing artifact size, while also hopefully improving consistency, and potentially improving performance. Separately, we're working on updating our vendored code to make updating it more repeatable (see #8391). This flagged a variety of modifications we'd made to the vendored System.Reflection.Metadata code to be able to use internal members. Rather than include those modifications in the updated vendored code, this PR updates our _existing_ usages to _not_ use those internal members in any scenario. This also means we can _stop_ using the vendored code for .NET Core 3.1+. ## Implementation details - Add pollyfil for `Hash` type used by Debugger code, which was internal to _System.Reflection.Metadata_ - Note that although this was called `FNV1aHash`, it _isn't_ using `FNV1a`, so I chose to call it simple hash (as that's what it's doing, a simple "hash code" calculation). - If these hashes _should_ be using FNV1A, then that should be addressed separately - Update namespaces to use the built-in versions where appropriate - These are intentionally _not_ added to `GlobalUsings.cs`, because doing so causes a variety of namespace collisions across our vendored code (Newtonsoft.Json, dnlib) - A couple of unused `using` statements needed removing. - Stop using internal members on `MethodDefinition` - In some cases, there are simple replacements available, e.g. replacing `FromRowId`. - Removing usages of `MethodDefinition.Handle` was a little more painful, as it required more refactoring, but fundamentally we were always deriving a `MethodDefinition` _from_ a `MethodDefinitionHandle`, so the changes are mostly a minor inconvenience, but they now only use public APIs. ## Test coverage Should be covered by existing tests; all these changes are just "refactorings", nothing additional. ### Assembly size comparison | Runtime | Before | After | Diff | % |-----------------|----------|----------|---------|------- | `netcoreapp3.1` | 7,919.5 KB | 7,484.0 KB | -435.5 KB | -5.50% | `net6.0` | 8,016.5 KB | 7,581.5 KB | -435 KB | -5.43% ### Estimated size per namespace #### Before (`net6.0`) ``` Total 7.15 MB (100.0%) └── Datadog 7.08 MB (99.1%) └── Trace 7.08 MB (99.1%) └── VendoredMicrosoftCode 0.54 MB (7.5%) └── System 0.54 MB (7.5%) └── Reflection 0.52 MB (7.3%) ├── Metadata 0.42 MB (5.8%) ├── PortableExecutable 0.07 MB (1.0%) └── Internal 0.03 MB (0.4%) ``` #### After (`net6.0`) (all gone) ``` Total 6.72 MB (100.0%) └── Datadog 6.66 MB (99.1%) └── Trace 6.66 MB (99.1%) ``` ## Other details https://datadoghq.atlassian.net/browse/APMLP-1207 Part of a stack updating our vendored system code - #8391
andrewlock
added a commit
that referenced
this pull request
Apr 22, 2026
## Summary of changes - Remove some unused types from System.Reflection.Metadata (mostly around builder/writer infrastructure that was unused) - Update the vendored version of System.Reflection.Metadata to use the runtime repo directly, and bump to latest patch version ## Reason for change We want to update our vendored .NET library versions. ## Implementation details - Make the vendoring repeatable - Run the tool, check for errors, tweak, rinse and repeat - After running the tool, used 🤖 to identify segments of code that weren't used, so we could strip them out ## Test coverage This is the test, if it compiles and tests pass, we should be ok 🤞 ## Other details https://datadoghq.atlassian.net/browse/APMLP-1207 Part of a stack updating our vendored system code - #8391 - #8454
| <!-- https://www.nuget.org/packages/System.Collections.Immutable/7.0.0 --> | ||
| <PackageReference Include="System.Collections.Immutable" Version="7.0.0" /> | ||
| <!-- https://www.nuget.org/packages/System.Collections.Immutable/7.0.20 --> | ||
| <PackageReference Include="System.Collections.Immutable" Version="7.0.20" /> |
Member
There was a problem hiding this comment.
I assume there's a reason we can't go higher than 7.x?
Member
Author
There was a problem hiding this comment.
Not necessarily, but potentially 😄 The important thing for me was to get the update process repeatable, and get on the latest of the available patches. Subsequently, we can look at bumping this (and others) higher, in follow ups 🙂
andrewlock
added a commit
that referenced
this pull request
Apr 23, 2026
## Summary of changes - Update the System.Memory based on NuGet package sources ## Reason for change We want to update our vendored .NET library versions. ## Implementation details - Make the vendoring repeatable - Run the tool, check for errors, tweak, rinse and repeat - After running the tool, used 🤖 to identify segments of code that weren't used, so we could strip them out The updated vendoring is based on the public [System.Memory](https://www.nuget.org/packages/System.Memory/4.6.3#dependencies-body-tab) nuget package, so is designed to be used with .NET Framework and .NET Standard, so makes the most sense to use IMO. As this package also uses _System.Buffers_ and _System.Numerics.Vectors_, vendored those pieces we need where appropriate. ## Test coverage This is the test, if it compiles and tests pass, we should be ok 🤞 ## Other details https://datadoghq.atlassian.net/browse/APMLP-1207 Note that currently, there's a lot of `Utf8Formatter` code that _isn't_ used, and could be excluded, however, given that theoretically we could/should use this in the future. I'm torn whether to just leave it in, or whether to tear it out for now, and restore it if/when we want to use it later. Any thoughts? Part of a stack updating our vendored system code - #8391 - #8454 - #8455
andrewlock
added a commit
that referenced
this pull request
Apr 23, 2026
## Summary of changes Stop vendoring the `SR` + regex files for microsoft code ## Reason for change Using `ResourceManager` is overkill, as we don't deploy all the translations etc anyway, and don't want to. ## Implementation details Replaced all the `SR.` accesses in previous PRs, so this is now dead code that we can remove. ## Test coverage If it builds, we're good ## Other details https://datadoghq.atlassian.net/browse/APMLP-1207 Note that currently, there's a lot of `Utf8Formatter` code that _isn't_ used, and could be excluded, however, given that theoretically we could/should use this in the future. I'm torn whether to just leave it in, or whether to tear it out for now, and restore it if/when we want to use it later. Any thoughts? Part of a stack updating our vendored system code - #8391 - #8454 - #8455 - #8459
andrewlock
added a commit
that referenced
this pull request
Apr 23, 2026
## Summary of changes - Create re-usable vendoring process for System.Runtime.CompilerServices.Unsafe - Update the vendored code (it's actually unchanged) ## Reason for change We want to be able to update vendored code as required. For the vendored [System.Runtime.CompilerServices.Unsafe package](https://nuget.info/packages/System.Runtime.CompilerServices.Unsafe/6.1.2), the [source code](https://github.com/dotnet/maintenance-packages/blob/14e29655e53aec37342e933bfd7ba574167453ff/src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.il) is written directly as IL, and [compiled using Ilasm.exe](https://learn.microsoft.com/en-us/dotnet/framework/tools/ilasm-exe-il-assembler). So this PR introduces a simple "IL to C#" converter that converts that file to its C# equivalent, using the same `InlineIL.Fody` approach that we currently have. The final result produces a file that is _virtually_ identical to the existing one (which is good!). The only difference I made was to add the original IL line as a comment next to the Fody equivalent. This also shows that the code has not _actually_ changed (and it's unlikely it _will_ tbh), so this just means we have a repeatable self-contained approach to regenerate this in the repo as required. ## Implementation details Told 🤖 to make the IL to C# converter, and it did 😄 I've given the code it generated a once-over to look for anything terrible, but the key thing is that the _output_ is sane, and that's visibly basically unchanged, so I think it's fine. ## Test coverage @dudikeleti already wrote some extensive tests for the `Unsafe` implementation back when he originally ported it, which verifies that we can compile the methods, call them, and that the generated IL is identical to the "real" versions 🎉 ## Other details https://datadoghq.atlassian.net/browse/APMLP-1207 Part of a stack updating our vendored system code - #8391 - #8454 - #8455 - #8459 - #8461
andrewlock
added a commit
that referenced
this pull request
Apr 24, 2026
#8476) ## Summary of changes Updates the System.Memory vendoring code to move types like `ReadOnlySpan<T>`, `Span<T>` into `System` instead of `Datadog.Trace.VendoredMicrosoftCode.System` ## Reason for change The compiler has various functionality that relies on the `Span<T>` (and `ReadOnlySpan<T>`) being available in the `System` namespace. 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 - #8391 - #8454 - #8455 - #8459 - #8461 - #8469
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) <[email protected]>
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
4 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
Reason for change
We want to update our vendored .NET library versions.
Implementation details
Test coverage
This is the test, if it compiles, we should be ok 🤞
Other details
https://datadoghq.atlassian.net/browse/APMLP-1207