Skip to content

Conversation

@ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Aug 5, 2025

CSharpDeclarationComputer.ComputeDeclarations shows up as 1.0% of Roslyn OOP allocations during a completion scenario in the RazorEditingTests.CompletionInCohosting speedometer test. The majority of the allocations under this method appear to be linq related, so switching to the model where we use a pooled collection to collect the items rather than linq enumerables.

Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/659161

*** Before this change ***
image

*** With this change ***
image

This is a draft until I get back numbers from a speedometer run.

CSharpDeclarationComputer.ComputeDeclarations shows up as 1.0% of Roslyn OOP allocations during a completion scenario in the RazorEditingTests.CompletionInCohosting speedometer test. The majority of the allocations under this method appear to be linq related, so switching to the model where we use a pooled collection to collect the items rather than linq enumerables.
@ToddGrun ToddGrun requested a review from a team as a code owner August 5, 2025 17:35
@333fred 333fred marked this pull request as draft August 5, 2025 18:07
…e more obvious after looking at a test run with changes from the first commit
@ToddGrun ToddGrun changed the title *** WIP: Reduce allocations in DeclataionComputer *** WIP: Reduce allocations in DeclarationComputer Aug 6, 2025
@ToddGrun ToddGrun marked this pull request as ready for review August 6, 2025 13:57
@ToddGrun ToddGrun changed the title *** WIP: Reduce allocations in DeclarationComputer Reduce allocations in DeclarationComputer Aug 6, 2025
@ToddGrun
Copy link
Contributor Author

@dotnet/roslyn-compiler -- ptal

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (commit 2)

@jcouv
Copy link
Member

jcouv commented Aug 25, 2025

@dotnet/roslyn-compiler for second review. Thanks

@jcouv jcouv self-assigned this Aug 25, 2025
var codeBlocks = ImmutableArray<SyntaxNode>.Empty;
if (executableCodeBlocks != null)
{
executableCodeBlocks.RemoveAll(c => c == null);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nicer to use AddIfNotNull at the callsites, so taht there can't be nulls in here in the first place. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into it, and there are quite a few callers, so I don't think I want to mess with that in this PR.

@ToddGrun ToddGrun merged commit 6669099 into dotnet:main Aug 28, 2025
28 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 28, 2025
@akhera99 akhera99 modified the milestones: Next, 18.0 P1, 18.0 P2 Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants