CompletionService refactoring and small tweaks to import completion#59125
CompletionService refactoring and small tweaks to import completion#59125genlu merged 22 commits intodotnet:mainfrom
Conversation
And minor cleanup in CompletionService APIs where CompletionList is used
by setting capacity of the builder to avoid call to EnsureCapacity
Instead of passing in a boolean to indicated if its triggered by selecting expander button, now import completion providers only need to check if import completion is enabled.
Was broken by previous refactoring
So we can always wait for unimported items and force index creation
64565d3 to
a83e943
Compare
a83e943 to
217ae3b
Compare
| private readonly record struct VSCompletionItemData | ||
| (string DisplayText, ImageElement Icon, ImmutableArray<AsyncCompletionData.CompletionFilter> Filters, | ||
| int FilterSetData, ImmutableArray<ImageElement> AttributeIcons, string InsertionText); |
There was a problem hiding this comment.
move to separate file. my pref on formatting would be:
private readonly record struct VSCompletionItemData(
string DisplayText,
ImageElement Icon,
ImmutableArray<AsyncCompletionData.CompletionFilter> Filters,
int FilterSetData,
ImmutableArray<ImageElement> AttributeIcons,
string InsertionText);
Taht way all the properties are very clear.
| Mask = mask; | ||
| } | ||
| } | ||
| private readonly record struct FilterWithMask(CompletionFilter Filter, int Mask); |
There was a problem hiding this comment.
please move to separate file.
There was a problem hiding this comment.
That feels unnecessary to me, given this is a private, one-line auxiliary type. Is this required by our convention?
...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| using var _ = ArrayBuilder<CompletionItemWithHighlight>.GetInstance(items.Count, out var builder); | ||
| builder.AddRange(items.Select(matchResult => |
There was a problem hiding this comment.
why not use SleectAsArray? i'm not sure how this new code is better.
There was a problem hiding this comment.
items is a pooled list, not an ImmutableArray, I think the overload of SelectAsArray for it doesn't create the instance of ArrayBuilder with a capacity. Give the potentially large size of this list, this would avoid call to EnsureCapacity thus reduce allocation.
There was a problem hiding this comment.
note that eventually we'd want to avoid potentially LOH allocation here, by asking for a new API from editor so we don't need to realize any ImmutableArray, and use pooled object instead.
src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/Completion/CompletionServiceWithProviders.ProviderManager.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public override async Task ProvideCompletionsAsync(CompletionContext completionContext) | ||
| { | ||
| // Don't trigger import completion if the option value is "default" and the experiment is disabled for the user. |
There was a problem hiding this comment.
what experiment do we still have here?
There was a problem hiding this comment.
Import completion is still disabled by default, we use an experiment to turn it on for all internal users.
| @@ -67,12 +66,10 @@ public Task WarmUpCacheAsync(Project? project, CancellationToken cancellationTok | |||
| s_cachingTask = Task.Run(() => WarmUpCacheAsync(workspace.CurrentSolution.GetProject(projectId), CancellationToken.None), CancellationToken.None); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
have you considered just using a AsyncBatchignWorkQueue instead?
There was a problem hiding this comment.
Will look into it, and change in a follow up PR if it's feasible.
|
i'm overall ok with this. my general feedback is similar to before. the refactorings are helping with complexity. but as you go inot methods have a strong eye toward doc'ing what is going on and making it so the methods are clearer in purpose and logic. Since you're doing several PRs this can be slowly done as you do all the work. but ideally the end result is that all the individual pieces feel clearer and far less complex when people are workign with them. thanks! |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
approved, with requests for some changes.
bb55230 to
26c2f4c
Compare
26c2f4c to
817f5d8
Compare
This is extracted from #59045.
Other than refactoring, there's also a few notable changes to the behavior of import completion (which lays the foundation for the additional changes in #59045):