Avoid unnecessary argument-builder work in generic type substitution - #85230
Merged
Conversation
Defer argument builder creation until the first changed argument and reuse immutable arguments when only the containing type changes. Add focused regression coverage and substitution benchmarks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Member
Author
|
@dotnet/roslyn-compiler PTAL |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues were identified.
Review tier: Lite
Findings: None
What changed in this PR
This pull request optimizes generic type substitution by lazily rebuilding type arguments and reusing unchanged arrays.
Changes:
- Defers argument-builder allocation until an argument changes.
- Adds substitution regression tests.
- Adds benchmarks and required internals access.
| File | Description |
|---|---|
src/Tools/Benchmarks/GenericTypeSubstitutionBenchmarks.cs |
Adds performance scenarios. |
src/Compilers/CSharp/Test/Symbol/Symbols/Source/TypeMapTests.cs |
Adds substitution regression coverage. |
src/Compilers/CSharp/Portable/Symbols/AbstractTypeMap.cs |
Implements lazy argument construction and reuse. |
src/Compilers/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.csproj |
Grants benchmark access to C# internals. |
src/Compilers/Core/Portable/Microsoft.CodeAnalysis.csproj |
Grants benchmark access to core internals. |
jjonescz
approved these changes
Sep 10, 2026
Member
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
This was referenced Sep 11, 2026
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
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.
Motivation
AbstractTypeMap.SubstituteNamedTypeeagerly rents and fills anArrayBuilder<TypeWithAnnotations>before determining whether substitution changes any arguments.The existing implementation avoids materializing an immutable array when nothing changes, but uses one
changedflag for two independent conditions: the containing type changes, or one or more type arguments change. This means unchanged arguments still get a new array when only the containing type changes.For example, consider a nested generic return type:
When binding
new Cache<int>().GetText(42), the compiler substitutesTKey → intinto the declared return type:The containing type changes, but
Entry's own argument array remains[string]. Outer type arguments are represented throughContainingType, not included in the inner type's own argument array.Previously, the containing-type change set
changed = true, bypassing theFree(); return previous;path and callingToImmutableAndFree(). The new symbol therefore received a newly materialized argument array even though the original immutable array could be reused.This is not a failure of pooling. The fully unchanged path already returned the builder to the pool and was allocation-free in steady state.
Change
Defer renting the builder until the first changed argument. At that point, copy the unchanged prefix and append the substituted arguments.
This separates two benefits: avoiding pool traffic and array writes for no-op substitutions, and avoiding an argument-array allocation for containing-type-only substitutions.
The change preserves the existing
TypeWithAnnotations.IsSameAscomparison, including nullable annotations and custom modifiers. Tuple handling is unchanged. It does not change alpha renaming or how substituted containing types are constructed.Performance
BenchmarkDotNet measurements on .NET 10.0.11, Windows, Ryzen 9 5950X. Baseline and candidate used saved Release builds and the in-process toolchain to avoid rebuilding the baseline against modified sources. Stronger runs used candidate then baseline order, six warmup iterations and 15 measurement iterations with a 250 ms iteration time.
No-op execution time decreases approximately 29–31%, with zero steady-state allocation before and after. Containing-type-only cases save 48, 72, and 216 bytes per operation, respectively. Changed-argument cases are not uniformly faster; two cases have approximately 2% slower point estimates with overlapping error intervals.
For a Replay workload containing 1,648 compiler requests, sampled allocation attribution for C#
TypeWithAnnotations[]decreased from 1.78 GiB to 1.67 GiB, approximately 6.3%. These are type-wide sampled estimates, not allocations attributed exclusively to this method. The trace reader reported invalid negative lost-event counters, so event loss is unknown.Full-workload timings remain inconclusive: baseline runs took 184/163 seconds and candidate runs took 156/176 seconds in baseline/candidate/candidate/baseline order. No end-to-end compilation speedup is claimed.
Coverage
Adds 14 benchmark cases covering no-op, first/last changed arguments, containing-type-only changes, and nested substitutions. Grants the existing
Benchmarksassembly access to compiler internals for these measurements.Adds 18 regression cases covering symbol identity, nested types, tuple names, nullable annotations, and custom modifiers. The original algorithm passed all 22 TypeMap cases; the candidate passed 34 focused/related cases across TypeMapTests and NoPiaLocalHideAndTypeSubstitutionTests. Release Replay and Benchmarks builds passed with analyzers enabled.
All four Replay runs completed 1,648 compilations successfully, and all 2,164 emitted DLL/EXE/PDB files matched byte-for-byte across the four runs.
Performance-tracing documentation is intentionally deferred to a separate change.
Microsoft Reviewers: Open in CodeFlow