Reduce allocations in AbstractMoveToNamespaceService#82837
Reduce allocations in AbstractMoveToNamespaceService#82837olegtk wants to merge 1 commit intodotnet:mainfrom
Conversation
dotnet-policy-service
bot
added
Community
|
/pr-val |
|
You do not have permission to trigger this workflow. Only Microsoft employees who are contributors to the roslyn repository can run pipelines. |
|
Failed to trigger the pipeline. Please check the workflow logs for details. |
|
/pr-val |
|
You do not have permission to trigger this workflow. Only Microsoft employees who are contributors to the roslyn repository can run pipelines. |
|
Failed to trigger the pipeline. Please check the workflow logs for details. |
|
/pr-val |
|
This PR is from an external author. You must specify a commit hash to trigger this workflow. Please use the format |
|
Failed to trigger the pipeline. Please check the workflow logs for details. |
|
/pr-val 1f8f220 |
|
View PR Validation Run triggered by @ToddGrun Parameters
|
| => node.AncestorsAndSelf().OfType<TNamespaceDeclarationSyntax>().Count() + node.DescendantNodes().OfType<TNamespaceDeclarationSyntax>().Count(); | ||
| { | ||
| var count = GetAncestorNamespaceCount(node); | ||
| foreach (var descendant in node.DescendantNodes()) |
There was a problem hiding this comment.
This still seems quite expensive if we're just trying to count the number of nodes, since we'll still realize all sorts of red nodes, right? Or put another way I'm not worried about the LINQ here but rather this algorithm just seems not great.
There was a problem hiding this comment.
This almost fits in with Cyrus's PR, if this could send in a greenfilter that would verify the greennode is of an appropriate type.
| result.Add(GetQualifiedName(ns)); | ||
| } | ||
|
|
||
| return result.ToImmutableAndClear(); |
There was a problem hiding this comment.
return compilation.GlobalNamespace.GetAllNamespaces(cancellationToken)
.SelectAsArray(
predicate: n => n.NamespaceKind == NamespaceKind.Module && n.ContainingAssembly == compilation.Assembly,
selector: GetQualifiedName);
bonus points if you add an overload that takes in an arg and make the predicate static
| /// <summary> | ||
| /// Counts namespace declarations in ancestor nodes (including self) only. | ||
| /// This avoids the expensive descendant traversal when the node is inside a type | ||
| /// declaration (which cannot contain namespace declarations). |
There was a problem hiding this comment.
I'm not clear on this comment, seems like something that is commenting the caller's behavior?
|
|
||
| private static bool ContainsMultipleTypesInSpine(SyntaxNode node) | ||
| => node.AncestorsAndSelf().OfType<TNamedTypeDeclarationSyntax>().Count() > 1; | ||
| { |
There was a problem hiding this comment.
This is very unlikely to be expensive
| /// This avoids the expensive descendant traversal when the node is inside a type | ||
| /// declaration (which cannot contain namespace declarations). | ||
| /// </summary> | ||
| private static int GetAncestorNamespaceCount(SyntaxNode node) |
There was a problem hiding this comment.
Using linq here is very unlikely to be expensive
| => node.AncestorsAndSelf().OfType<TNamespaceDeclarationSyntax>().Count() + node.DescendantNodes().OfType<TNamespaceDeclarationSyntax>().Count(); | ||
| { | ||
| var count = GetAncestorNamespaceCount(node); | ||
| foreach (var descendant in node.DescendantNodes()) |
There was a problem hiding this comment.
The problem seems to be here. We just need to walk down namespaces. This should bottom out almost immediately without a full walk.
4 participants
*** Not ready for review until perf numbers come back ***
Reduce allocations in AbstractMoveToNamespaceService that appear in CopilotCompletions.NextSuggestion Speedometer test.
Microsoft Reviewers: Open in CodeFlow