Reduce allocations in CSharpSyntaxNode.GetStructure #80562
Merged
Conversation
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
WIP until I get back speedometer numbers. If those look good, then will consider elevating out of draft status and add more details.
src/Compilers/CSharp/Portable/Syntax/InternalSyntax/CSharpSyntaxNode.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Syntax/InternalSyntax/CSharpSyntaxNode.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
approved these changes
Oct 5, 2025
Member
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Approving either approach. Though i prefer the smalldict approach just as being good for the single item case, and for not having to worry about linear scaling.
Contributor
Author
Updated the numbers in the description. Going ahead with the SmallDictionary version, as it's showing slightly better allocation numbers, but slightly worse CPU usage. It's simpler, and you've expressed a preference, so I'm good with that. |
ToddGrun
changed the title
Contributor
Author
|
@dotnet/roslyn-compiler -- ptal |
333fred
approved these changes
Oct 6, 2025
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Oct 7, 2025
* upstream/main: (252 commits) Move Watch EA to a separate assembly Microsoft.CodeAnalysis.ExternalAccess.HotReload (dotnet#80556) Enable long paths for Windows DartLab CI (dotnet#80581) Ensure that CS8659 is reported on partial properties (dotnet#80573) Fix a wrong relative link in a doc (dotnet#80567) [main] Source code updates from dotnet/dotnet (dotnet#80578) Update dependencies from https://github.com/dotnet/arcade build 20251006.2 (dotnet#80577) Update main configs after VS snap (dotnet#80523) Add followup async public apis (dotnet#80455) Reduce allocations in CSharpSyntaxNode.GetStructure (dotnet#80562) Extensions: Close some tracked follow-ups (dotnet#80527) Fix outdated 17.15 to 18.0 (dotnet#80570) Update dependencies from https://github.com/dotnet/dotnet build 285582 (dotnet#80551) Fix all-in-one tests [main] Update dependencies from dotnet/arcade (dotnet#80559) Improve error recovery when object initializer uses ':' instead of '=' (dotnet#80553) Support `field` keyword in EE. (dotnet#80515) Log a debug message for ContentModified exceptions. (dotnet#80549) Update dependencies from https://github.com/dotnet/arcade build 20251002.2 (dotnet#80550) Fix Simplify and add additional asserts ...
This was referenced Oct 8, 2025
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.
Reduce allocations in CSharpSyntaxNode.GetStructure
Change this method from using a Dictionary for each parent node to a SmallDictionary. There are typically very few entries in this collection, ~80 of the time it's a single item, ~95% of the time, it's two items. I've gone ahead with the SmallDictionary usage instead of a List<(key, value)> usage as it's a bit simpler and doesn't have a significant cost over the list implementation (slightly more CPU usage, but slightly less memory usage)
In addition to the data structure change, removed the usage of a WeakReference from inside the value in the CWT, as I don't think it's necessary.
Commit 1 test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/676546
Commit 2 test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/676592
Below is a commit level comparison of allocation and CPU costs in the html/C# completion scenarios of the razor cohosting speedometer test. Costs are in the CA process, under the CSharpSyntaxNode.GetStructure method. These numbers are the average over the five test runs. Absolute numbers vary quite a bit between runs, percentages are probably slightly more representative.