Cleanup the code we have to run copilot code in parallel #78324
Conversation
ghost
added
Area-IDE
untriaged
| internal sealed class CSharpContextProviderService([ImportMany] IEnumerable<IContextProvider> providers) | ||
| : ICSharpCopilotContextProviderService | ||
| { | ||
| // 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.
There was a problem hiding this comment.
oops, I forgot to move the tests over.
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
|
|
||
| <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.
There was a problem hiding this comment.
thanks! I initially tried to use roslyn's ProducerConsumer but it quickly ended up with the problem of requiring too much code to be exposed via EA, so I wrote my own parallelization instead. Now that we moved this code over, it makes perfect sense to fix it
| 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
|
@genlu ptal. |
3 participants
No description provided.