-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 2 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,36 @@ | |
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 | ||
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: 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 commentThe reason will be displayed to describe this comment to others. Learn more. oops, I forgot to move the tests over. |
||
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>.RunAsync( | ||
static (callback, args, cancellationToken) => | ||
RoslynParallel.ForEachAsync( | ||
CyrusNajmabadi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
args.@this._providers, | ||
cancellationToken, | ||
(provider, cancellationToken) => provider.ProvideContextItemsAsync( | ||
CyrusNajmabadi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
args.document, args.position, args.activeExperiments, | ||
(items, cancellationToken) => | ||
{ | ||
foreach (var item in items) | ||
callback(item); | ||
|
||
return default; | ||
}, cancellationToken)), | ||
args: (@this: this, 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> | ||
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(); | ||
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. 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 |
||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.