-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Cleanup the code we have to run copilot code in parallel #78324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0bdfc94
39b008e
dcb497c
925e97e
a978dea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,80 +6,34 @@ | |||||||
| using System.Collections.Generic; | ||||||||
| using System.Collections.Immutable; | ||||||||
| using System.Composition; | ||||||||
| using System.Linq; | ||||||||
| using System.Runtime.CompilerServices; | ||||||||
| using System.Threading; | ||||||||
| using System.Threading.Tasks; | ||||||||
| using Microsoft.CodeAnalysis; | ||||||||
| using Microsoft.CodeAnalysis.ErrorReporting; | ||||||||
| using Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion; | ||||||||
| using Microsoft.CodeAnalysis.Host.Mef; | ||||||||
| using Microsoft.VisualStudio.Threading; | ||||||||
| using Microsoft.CodeAnalysis.Shared.Utilities; | ||||||||
|
|
||||||||
| namespace Microsoft.CodeAnalysis.ExternalAccess.Copilot.Internal.Completion; | ||||||||
|
|
||||||||
| [Shared] | ||||||||
| [Export(typeof(ICSharpCopilotContextProviderService))] | ||||||||
| internal sealed class CSharpContextProviderService : ICSharpCopilotContextProviderService | ||||||||
| [Export(typeof(ICSharpCopilotContextProviderService)), Shared] | ||||||||
| [method: ImportingConstructor] | ||||||||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||||||||
| internal sealed class CSharpContextProviderService([ImportMany] IEnumerable<IContextProvider> providers) | ||||||||
| : ICSharpCopilotContextProviderService | ||||||||
| { | ||||||||
| // Exposed for testing | ||||||||
| public ImmutableArray<IContextProvider> Providers { get; } | ||||||||
|
|
||||||||
| [ImportingConstructor] | ||||||||
| [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||||||||
| public CSharpContextProviderService([ImportMany] IEnumerable<IContextProvider> providers) | ||||||||
| { | ||||||||
| Providers = providers.ToImmutableArray(); | ||||||||
| } | ||||||||
|
|
||||||||
| public async IAsyncEnumerable<IContextItem> GetContextItemsAsync(Document document, int position, IReadOnlyDictionary<string, object> activeExperiments, [EnumeratorCancellation] CancellationToken cancellationToken) | ||||||||
| { | ||||||||
| var queue = new AsyncQueue<IContextItem>(); | ||||||||
| var tasks = this.Providers.Select(provider => Task.Run(async () => | ||||||||
| { | ||||||||
| try | ||||||||
| { | ||||||||
| await provider.ProvideContextItemsAsync(document, position, activeExperiments, ProvideItemsAsync, cancellationToken).ConfigureAwait(false); | ||||||||
| } | ||||||||
| catch (Exception exception) when (FatalError.ReportAndCatchUnlessCanceled(exception, ErrorSeverity.General)) | ||||||||
| { | ||||||||
| } | ||||||||
| }, | ||||||||
| cancellationToken)); | ||||||||
|
|
||||||||
| // Let all providers run in parallel in the background, so we can steam results as they come in. | ||||||||
| // Complete the queue when all providers are done. | ||||||||
| _ = Task.WhenAll(tasks) | ||||||||
| .ContinueWith((_, __) => queue.Complete(), | ||||||||
| null, | ||||||||
| cancellationToken, | ||||||||
| TaskContinuationOptions.ExecuteSynchronously, | ||||||||
| TaskScheduler.Default); | ||||||||
|
|
||||||||
| while (true) | ||||||||
| { | ||||||||
| IContextItem item; | ||||||||
| try | ||||||||
| { | ||||||||
| item = await queue.DequeueAsync(cancellationToken).ConfigureAwait(false); | ||||||||
| } | ||||||||
| catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) | ||||||||
| { | ||||||||
| // Dequeue is cancelled because the queue is empty and completed, we can break out of the loop. | ||||||||
| break; | ||||||||
| } | ||||||||
|
|
||||||||
| yield return item; | ||||||||
| } | ||||||||
|
|
||||||||
| ValueTask ProvideItemsAsync(ImmutableArray<IContextItem> items, CancellationToken cancellationToken) | ||||||||
| { | ||||||||
| foreach (var item in items) | ||||||||
| { | ||||||||
| queue.Enqueue(item); | ||||||||
| } | ||||||||
|
|
||||||||
| return default; | ||||||||
| } | ||||||||
| } | ||||||||
| private readonly ImmutableArray<IContextProvider> _providers = [.. providers]; | ||||||||
|
|
||||||||
| public IAsyncEnumerable<IContextItem> GetContextItemsAsync(Document document, int position, IReadOnlyDictionary<string, object> activeExperiments, CancellationToken cancellationToken) | ||||||||
| => ProducerConsumer<IContextItem>.RunParallelStreamAsync( | ||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||
| _providers, | ||||||||
| static async (provider, callback, args, cancellationToken) => | ||||||||
| await provider.ProvideContextItemsAsync( | ||||||||
| args.document, args.position, args.activeExperiments, | ||||||||
| (items, cancellationToken) => | ||||||||
| { | ||||||||
| foreach (var item in items) | ||||||||
| callback(item); | ||||||||
|
|
||||||||
| return default; | ||||||||
| }, cancellationToken).ConfigureAwait(false), | ||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: if ProvideContextItemsAsync just took an Action this could would be simplified to:
Suggested change
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. |
||||||||
| args: (document, position, activeExperiments), | ||||||||
| cancellationToken); | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,10 +30,6 @@ | |
| <ProjectReference Include="..\..\Core\Portable\Microsoft.CodeAnalysis.Features.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.VisualStudio.Threading" /> | ||
| </ItemGroup> | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. definitely do not want this. |
||
|
|
||
| <ItemGroup> | ||
| <PublicAPI Include="PublicAPI.Shipped.txt" /> | ||
| <PublicAPI Include="PublicAPI.Unshipped.txt" /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,34 +2,29 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using System.Composition; | ||
| using Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion; | ||
| using Microsoft.CodeAnalysis.PooledObjects; | ||
| using Microsoft.CodeAnalysis.Text; | ||
| using Roslyn.LanguageServer.Protocol; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Copilot; | ||
|
|
||
| [Shared] | ||
| [Method(MethodName)] | ||
| [ExportCSharpVisualBasicStatelessLspService(typeof(CopilotCompletionResolveContextHandler), WellKnownLspServerKinds.Any)] | ||
| internal sealed class CopilotCompletionResolveContextHandler : ILspServiceDocumentRequestHandler<ContextResolveParam, IContextItem[]> | ||
| [ExportCSharpVisualBasicStatelessLspService(typeof(CopilotCompletionResolveContextHandler), WellKnownLspServerKinds.Any), Shared] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete("This exported object must be obtained through the MEF export provider.", error: true)] | ||
| internal sealed class CopilotCompletionResolveContextHandler(ICSharpCopilotContextProviderService contextProviderService) | ||
| : ILspServiceDocumentRequestHandler<ContextResolveParam, IContextItem[]> | ||
| { | ||
| // "@2" prefix to differentiate it from the implementation previously located in devkit extension. | ||
| private const string MethodName = "roslyn/resolveContext@2"; | ||
|
|
||
| [ImportingConstructor] | ||
| [Obsolete("This exported object must be obtained through the MEF export provider.", error: true)] | ||
| public CopilotCompletionResolveContextHandler(ICSharpCopilotContextProviderService contextProviderService) | ||
| { | ||
| ContextProviderService = contextProviderService; | ||
| } | ||
|
|
||
| public bool MutatesSolutionState => false; | ||
|
|
||
| public bool RequiresLSPSolution => true; | ||
|
|
||
| public ICSharpCopilotContextProviderService ContextProviderService { get; } | ||
| public ICSharpCopilotContextProviderService ContextProviderService { get; } = contextProviderService; | ||
|
|
||
| public TextDocumentIdentifier GetTextDocumentIdentifier(ContextResolveParam request) | ||
| => request.DocumentContext.TextDocument; | ||
|
|
@@ -41,13 +36,11 @@ public async Task<IContextItem[]> HandleRequestAsync(ContextResolveParam param, | |
|
|
||
| var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false); | ||
| var position = text.Lines.GetPosition(linePosition); | ||
| var builder = ImmutableArray.CreateBuilder<IContextItem>(); | ||
| var activeExperiments = param.GetUnpackedActiveExperiments(); | ||
|
|
||
| using var _ = ArrayBuilder<IContextItem>.GetInstance(out var builder); | ||
| await foreach (var item in ContextProviderService.GetContextItemsAsync(document, position, activeExperiments, cancellationToken).ConfigureAwait(false)) | ||
| { | ||
| builder.Add(item); | ||
| } | ||
|
|
||
| return builder.ToArray(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea, I was planning to do this, just didn'r have time to figure out how to return an AyncIterable yet |
||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, I forgot to move the tests over.