Modify ISyntaxFacts methods to allocate less #74596
Merged
Conversation
This file contains 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
CSharpSyntaxFacts.GetMethodLevelMembers shows up as 3.4% of allocations in the scrolling speedometer test during the typing phase of the test. This method (and it's partner GetTopLevelAndMethodLevelMembers) can be easily made to take in and populate an arraybuilder instead of allocating a list on each invocation.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
CyrusNajmabadi
approved these changes
Jul 30, 2024
This was referenced Aug 7, 2024
ToddGrun
added a commit
to ToddGrun/roslyn
that referenced
this pull request
Aug 16, 2024
A recent change(dotnet#74596) modified this code to take in an ArrayBuilder to allow callers to easily use pooled arrays. However, the size of the data populated into those arrays commonly exceeds the ArrayBuilder reuse threshold, those reducing the allocation benefit realized in those codepaths. This PR changes the callers to instead use a pooled list, thus avoiding the threshold issue. Current scrolling speedometer results for the typing scenario still show this as allocating around 35 MB (which was about 3.5% at the time of the earlier PR). My hope is that this PR will get rid of nearly all those allocations this time.
ToddGrun
added a commit
that referenced
this pull request
Aug 21, 2024
* Reduce usage in CSharpSyntaxFacts.AppendMembers (again) A recent change(#74596) modified this code to take in an ArrayBuilder to allow callers to easily use pooled arrays. However, the size of the data populated into those arrays commonly exceeds the ArrayBuilder reuse threshold, those reducing the allocation benefit realized in those codepaths. This PR changes the callers to instead receive back a PooledObject<List<SyntaxNode>>, thus avoiding the threshold issue, and letting the callers release the List back to the pool at their own discretion. Current scrolling speedometer results for the typing scenario still show this as allocating around 35 MB (which was about 3.5% at the time of the earlier PR). Speedometer runs including this PR have none of those allocations.
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.
CSharpSyntaxFacts.GetMethodLevelMembers shows up as 3.4% of allocations in the scrolling speedometer test during the typing phase of the test. This method (and it's partner GetTopLevelAndMethodLevelMembers) can be easily made to take in and populate an arraybuilder instead of allocating a list on each invocation.
*** relevant allocation data from scrolling speedometer profile during typing part of the test ***
