Improvement to completion for unimported extension methods#45947
Improvement to completion for unimported extension methods#45947genlu merged 8 commits intodotnet:masterfrom
Conversation
Also returns partial results when not all indices are available
by aggregating overloads, also reduce the number of items returned
|
| { | ||
| } | ||
|
|
||
| private const string NonBreakingSpaceString = "\x00A0"; |
There was a problem hiding this comment.
This is the char used in the product, took me some debugging to figure out why my tests were failing.
...Features/Core/Portable/Completion/Providers/ImportCompletionProvider/ImportCompletionItem.cs
Outdated
Show resolved
Hide resolved
...Features/Core/Portable/Completion/Providers/ImportCompletionProvider/ImportCompletionItem.cs
Outdated
Show resolved
Hide resolved
...Features/Core/Portable/Completion/Providers/ImportCompletionProvider/ImportCompletionItem.cs
Outdated
Show resolved
Hide resolved
| builder.Add(MethodKey, extensionMethodData.Value.methodSymbolKey); | ||
| builder.Add(ReceiverKey, extensionMethodData.Value.receiverTypeSymbolKey); | ||
|
|
||
| if (extensionMethodData.Value.overloadCount > 0) |
There was a problem hiding this comment.
would maybe have been nice to have this in another pr.
There was a problem hiding this comment.
All changes related to this is in the last commit, skipping it would make it easier to review
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
|
|
||
| var indicesComplete = true; | ||
| using var _1 = PooledDictionary<INamespaceSymbol, string>.GetInstance(out var namespaceNameCache); | ||
| using var _2 = PooledDictionary<(string containingNamespace, string methodName, bool isGeneric), (IMethodSymbol bestSymbol, int overloadCount)>.GetInstance(out var overloadMap); |
There was a problem hiding this comment.
seriously on the fence about asking for some named structs.
There was a problem hiding this comment.
ValueTuple make it easier to use them as dictionary key
There was a problem hiding this comment.
understood. still on the fence :)
There was a problem hiding this comment.
When will we start dogfooding records? :)
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
|
my brain got tired 3/4ths through. i'll have to review the rest later. lmk if you're making more changes. |
Some quick measurement using telemetry events shows a 70% improvement over unparalleled version in average (on a 4 core machine). The test is done by repeatedly trigger and commit completion for members in EditorFeatures project. Note this is only based on the actual computation time, i.e. time spent in 'GetUnimportedExtensionMethodsInCurrentProcessAsync', which doesn't include the seemingly random overhead of OOP communication.
everything here is OOP
No more changes planned. |
Awesome. in terms of actual wallclock time, can you put that in perspective? i.e. MS before and MS after? Thanks! |
Great. No concerns about multithreading here then. |
In the scenario I tested, it's roughly 70 ms vs 210ms (average per completion session) |
That's fantastic! |
|
@CyrusNajmabadi Please let me know if you have more comments. Thanks! |
|
looking now. |
| ISet<string> namespaceFilter, | ||
| ConcurrentDictionary<ITypeSymbol, bool> checkedReceiverTypes, | ||
| StatisticCounter counter, | ||
| CancellationToken cancellationToken) |
There was a problem hiding this comment.
i would reocmmend in a future pr breaking this into a class. I think 10 params is def the trigger point that indicates things have gone too far.
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Outdated
Show resolved
Hide resolved
| // queue a new task only if the previous task is completed. This is to avoid | ||
| // queueing calculation for the same set of references repeatedly while | ||
| // index is being constrcuted, which might take some time. | ||
| if (s_indexingTask.IsCompleted) |
There was a problem hiding this comment.
i don't get the relation between teh indicesComplete flag and the s_indexingTask.
There was a problem hiding this comment.
indicesComplete is true means we have indices available for all references. When it's false, we would try to queue a background task to force create them, which is s_indexingTask, however, if s_indexingTask from a previous completion session is not completed, we will just let it finish w/o attaching another task to it.
...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs
Show resolved
Hide resolved
|
|
||
| if (index.ContainsExtensionMethod && parameters.SemanticModel.Compilation.GetAssemblyOrModuleSymbol(peReference) is IAssemblySymbol assembly) | ||
| { | ||
| var filter = CreateAggregatedFilter(parameters.ReceiverTypeNames, index); |
There was a problem hiding this comment.
the terms "filter" or "aggregated filter" aresn't really conveying to me what thsi is for.
There was a problem hiding this comment.
I have added some comments
Some quick measurement using telemetry events shows a 70% improvement over unparalleled version in average (on a 4 core machine). The test is done by repeatedly trigger and commit completion for members in EditorFeatures project. Note this is only based on the actual computation time, i.e. time spent in 'GetUnimportedExtensionMethodsInCurrentProcessAsync', which doesn't include the seemingly random overhead of OOP communication.
All the parallelized computation runs in OOP.