Improve AddImport conflict detection performance #73780
Conversation
We've recently obtained some traces from customers around ui delays during completion commit in C# files. The first profile I took a look at indicated that the delay was due to committing a snippet, and in particular during the code used to prevent adding conflicting imports. This code has been improved in 3 ways, all relating to accessing the semantic model (where the *vast* majority of the time was spent): 1) Reduce the number of nodes for which semantic information need be obtained. The code already collects a couple dictionaries keyed on type names which could cause conflicts. This simply changes the code to filter syntax nodes based on this. 2) Change the kind of semantic model used. About half the time used during the binding process was due to the model dealing with work around nullability. We don't need that infomation, so we're good to use the much faster nullabledisabled semantic model. 3) Process all the binding of nodes to their semantic info in parallel. This does a simple collection of nodes of interest, then uses the ProducerConsumer mechanism to obtain semantic information in parallel. As a minor tweak, I aslo did away with the TreeWalker derivation, instead just doing a simple walk over the results of GetDescendantNodes. Should be more performant, and I find it a bit easier to understand when the needs are this simple.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| break; | ||
| case SyntaxKind.SimpleMemberAccessExpression: | ||
| case SyntaxKind.PointerMemberAccessExpression: | ||
| if (IsPotentialConflictWithImportedExtensionMethod((MemberAccessExpressionSyntax)node)) |
There was a problem hiding this comment.
fwiw, i don't think the pointer case can happen with imports and extensions. but i don't mind you preserving this.
| // Track if we've seen an anonymous method or not. If so, because of how the language binds lambdas and | ||
| // overloads, we'll assume any method access we see inside (instance or otherwise) could end up conflicting | ||
| // with an extension method we might pull in. | ||
| containsAnonymousMethods = true; |
There was a problem hiding this comment.
ok to preserve this. but this genuinely makes no sense wrt to how the conflict checker has to work.
| // Drastically reduce the number of nodes that need to be inspected by filtering | ||
| // out nodes whose identifier isn't a potential conflict. | ||
| if (!_importedTypes.ContainsKey((node.Identifier.Text, node.Arity))) | ||
| return false; |
| cancellationToken).ConfigureAwait(False) | ||
|
|
||
| For Each conflict In items | ||
| conflicts.Add(conflict) |
There was a problem hiding this comment.
conflicts.AddRange(items)
There was a problem hiding this comment.
Yeah, probably easier that way. Just was in the habit of not doing that in the case where it would box an enumerator.
|
unit test failures -- will look into this tomorrow |
There was discussion about experiment with using nullable-disable for all of completion, wondering if this is something we are still planning to do |
We've recently obtained some traces from customers around UI delays during completion commit in C# files. The first profile I took a look at indicated that the delay was due to a snippet commit, in particular during the code used to prevent adding conflicting imports.
This code has been improved in 3 ways, all relating to accessing the semantic model (where the vast majority of the time was spent):
Reduce the number of nodes for which semantic information need be obtained. The code already collects a couple dictionaries keyed on type names which could cause conflicts. This simply changes the code to filter syntax nodes based on this.
Change the kind of semantic model used. About half the time used during binding was due to the model dealing with work around nullability. We don't need that information, so we're good to use the much faster NullableDisabled semantic model.
Process binding nodes to their semantic information in parallel. This does a simple collection of nodes of interest, then uses the ProducerConsumer mechanism to obtain semantic information in parallel.
As a minor tweak, I also did away with the TreeWalker derivation, instead just doing a simple walk over the results of GetDescendantNodes. Should be more performant, and I find it a bit easier to understand when the needs are this simple.