Cleanup the code we have to run copilot code in parallel #78324
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| { | ||
| // Exposed for testing |
There was a problem hiding this comment.
note: i couldn't find this actually read anywhere. if it is used for testing, it should be exposed through a GetTestAccessor() method.
src/Features/ExternalAccess/Copilot/Internal/Completion/CSharpContextProviderService.cs
Outdated
Show resolved
Hide resolved
src/Features/ExternalAccess/Copilot/Internal/Completion/CSharpContextProviderService.cs
Outdated
Show resolved
Hide resolved
| @@ -30,10 +30,6 @@ | |||
| <ProjectReference Include="..\..\Core\Portable\Microsoft.CodeAnalysis.Features.csproj" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="Microsoft.VisualStudio.Threading" /> | |||
| </ItemGroup> | |||
There was a problem hiding this comment.
definitely do not want this.
| builder.Add(item); | ||
| } | ||
|
|
||
| return builder.ToArray(); |
There was a problem hiding this comment.
this really should be using an IProgress value in LSP. That way this can stream the context items back to the caller, and so we can be canceled if we take too long, while also providing some items back that can be used within the time budgets of hte caller.
There was a problem hiding this comment.
yea, I was planning to do this, just didn'r have time to figure out how to return an AyncIterable yet
see: https://github.com/github/copilot-client/blob/fa8dce2647472b64bbad2128acb607d11811ead8/types/src/contextProviderApiV1.ts#L55
src/Features/ExternalAccess/Copilot/Internal/Completion/CSharpContextProviderService.cs
Outdated
Show resolved
Hide resolved
| private readonly ImmutableArray<IContextProvider> _providers = [.. providers]; | ||
|
|
||
| public IAsyncEnumerable<IContextItem> GetContextItemsAsync(Document document, int position, IReadOnlyDictionary<string, object> activeExperiments, CancellationToken cancellationToken) | ||
| => ProducerConsumer<IContextItem>.RunParallelStreamAsync( |
There was a problem hiding this comment.
this automatically parallelizes, prefering to utilize all cores to finish a fixed set of work that is then added to as work items complete. this is better than kicking off N items (which may be far mroe than the number of cores), causing them to contend with each other, and trying to .WhenAll them.
| callback(item); | ||
|
|
||
| return default; | ||
| }, cancellationToken).ConfigureAwait(false), |
There was a problem hiding this comment.
note: if ProvideContextItemsAsync just took an Action this could would be simplified to:
| }, cancellationToken).ConfigureAwait(false), | |
| (provider, cancellationToken) => provider.ProvideContextItemsAsync( | |
| document, position, activeExperiments, callback, cancellationToken); |
Which is much nicer and cleaner. It's unclear why we're passing a callback that has to package up everything in an array and be async.
|
@genlu ptal. Note: i would like to take this, and include inthe backport if possible. |
src/Features/ExternalAccess/Copilot/Completion/ICSharpCopilotContextProviderService.cs
Outdated
Show resolved
Hide resolved
…ontextProviderService.cs
No description provided.