Reduce allocations under SyntaxNodeExtensions.GetContent #11946
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
This method was creating a new StringWriter on every invocation. On net framework, each StringWriter incurs an additional char array allocation during construction. Instead, add a pool for stringwriters in GreenNode to avoid these allocations on every call. This reduced allocations spent under this method during the typing scenario in the razor lsp speedometer test from 3.5% to 1.9%. Note that it would be nice to have GreenNode.ToString use this optimization too, however it differed (intentionally?) from the caller in SyntaxNodeExtensions.GetContent by passing in the invariant culture to the StringWriter during construction. I don't see ToString called near as often in the profile I'm looking at, so, it didn't seem to warrant creating a separate pool for that method. I would appreciate verification though that it's intentional that these two calls use different culture semantics. Speedometer run: https://dev.azure.com/devdiv/DevDiv/_apps/hub/ms-vseng.pit-vsengPerf.pit-hub?targetBuild=10711.132.dn-bot.250612.052736.643013&targetBranch=main&targetPerfBuildId=11746547&runGroup=Speedometer&baselineBuild=10711.132&baselineBranch=main
ToddGrun
commented
Jun 12, 2025
| public static string GetContent<TNode>(this TNode node) where TNode : SyntaxNode | ||
| { | ||
| using var _ = StringBuilderPool.GetPooledObject(out var builder); | ||
| using var writer = new System.IO.StringWriter(builder); |
Contributor
Author
Member
There was a problem hiding this comment.
I really don't think you need to worry about CultureInfo for the StringWriter. GreenNode.WriteTo delegates to GreenNode.WriteTokenTo to do any actually writing. WriteTokenTo is only overridden by SyntaxToken and it just writes a string. IOW, nothing actually writes anything other than strings, so it's essentially culture-neutral.
Contributor
Author
There was a problem hiding this comment.
That's what I was hoping. Went ahead and changed to always use InvariantCulture. Thanks!
ToddGrun
commented
Jun 12, 2025
| return builder.ToString(); | ||
| } | ||
|
|
||
| internal string ToStringInCurrentCulture() |
Contributor
Author
There was a problem hiding this comment.
Right above here is where ToString instead specifies InvariantCulture.
If these were to match, the ToStringInCurrentCulture change could be moved to ToString.
DustinCampbell
approved these changes
Jun 12, 2025
333fred
approved these changes
Jun 12, 2025
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.
This method was creating a new StringWriter on every invocation. On net framework, each StringWriter incurs an additional char array allocation during construction. Instead, add a pool for stringwriters in GreenNode to avoid these allocations on every call. This reduced allocations spent under this method during the typing scenario in the razor lsp speedometer test from 3.5% to 1.9%.
Note that it would be nice to have GreenNode.ToString use this optimization too, however it differed (intentionally?) from the caller in SyntaxNodeExtensions.GetContent by passing in the invariant culture to the StringWriter during construction. I don't see ToString called near as often in the profile I'm looking at, so, it didn't seem to warrant creating a separate pool for that method. I would appreciate verification though that it's intentional that these two calls use different culture semantics.
Speedometer run: https://dev.azure.com/devdiv/DevDiv/_apps/hub/ms-vseng.pit-vsengPerf.pit-hub?targetBuild=10711.132.dn-bot.250612.052736.643013&targetBranch=main&targetPerfBuildId=11746547&runGroup=Speedometer&baselineBuild=10711.132&baselineBranch=main
Allocations before change
Allocations with change
