Skip to content
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

Delay starting the work to scan for todo-items #60654

Merged
merged 5 commits into from
Apr 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/EditorFeatures/Core/TodoComment/TodoItemsUpdatedArgs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ internal sealed class TodoItemsUpdatedArgs : UpdatedEventArgs
public ImmutableArray<TodoCommentData> TodoItems { get; }

public TodoItemsUpdatedArgs(
object id, Workspace workspace, Solution solution, ProjectId projectId, DocumentId documentId, ImmutableArray<TodoCommentData> todoItems)
: base(id, workspace, projectId, documentId)
object id, Solution solution, DocumentId documentId, ImmutableArray<TodoCommentData> todoItems)
: base(id, solution.Workspace, documentId.ProjectId, documentId)
{
Solution = solution;
TodoItems = todoItems;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.SolutionCrawler;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ namespace Microsoft.CodeAnalysis.TodoComments
{
internal sealed class InProcTodoCommentsIncrementalAnalyzer : AbstractTodoCommentsIncrementalAnalyzer
{
private readonly ITodoCommentsListener _listener;
private readonly TodoCommentsListener _listener;

public InProcTodoCommentsIncrementalAnalyzer(ITodoCommentsListener listener)
public InProcTodoCommentsIncrementalAnalyzer(TodoCommentsListener listener)
=> _listener = listener;

protected override ValueTask ReportTodoCommentDataAsync(DocumentId documentId, ImmutableArray<TodoCommentData> data, CancellationToken cancellationToken)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ namespace Microsoft.CodeAnalysis.TodoComments
/// </remarks>
internal sealed class InProcTodoCommentsIncrementalAnalyzerProvider : IIncrementalAnalyzerProvider
{
private readonly ITodoCommentsListener _listener;
private readonly TodoCommentsListener _listener;

public InProcTodoCommentsIncrementalAnalyzerProvider(ITodoCommentsListener listener)
public InProcTodoCommentsIncrementalAnalyzerProvider(TodoCommentsListener listener)
=> _listener = listener;

public IIncrementalAnalyzer CreateIncrementalAnalyzer(Workspace workspace)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ internal sealed class TodoCommentsListener : ITodoCommentsListener, IDisposable
/// <summary>
/// Queue where we enqueue the information we get from OOP to process in batch in the future.
/// </summary>
private readonly TaskCompletionSource<AsyncBatchingWorkQueue<DocumentAndComments>> _workQueueSource = new();
private readonly AsyncBatchingWorkQueue<DocumentAndComments> _workQueue;

public TodoCommentsListener(
IGlobalOptionService globalOptions,
Expand All @@ -50,6 +50,12 @@ public TodoCommentsListener(
_asyncListener = asynchronousOperationListenerProvider.GetListener(FeatureAttribute.TodoCommentList);
_onTodoCommentsUpdated = onTodoCommentsUpdated;
_disposalToken = disposalToken;

_workQueue = new AsyncBatchingWorkQueue<DocumentAndComments>(
TimeSpan.FromSeconds(1),
ProcessTodoCommentInfosAsync,
_asyncListener,
_disposalToken);
}

public void Dispose()
Expand All @@ -64,14 +70,10 @@ public void Dispose()

public async ValueTask StartAsync()
{
var cancellationToken = _disposalToken;
// Should only be started once.
Contract.ThrowIfTrue(_lazyConnection != null);

_workQueueSource.SetResult(
new AsyncBatchingWorkQueue<DocumentAndComments>(
TimeSpan.FromSeconds(1),
ProcessTodoCommentInfosAsync,
_asyncListener,
cancellationToken));
var cancellationToken = _disposalToken;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this was lazily created.


var client = await RemoteHostClient.TryGetClientAsync(_services, cancellationToken).ConfigureAwait(false);
if (client == null)
Expand Down Expand Up @@ -120,12 +122,12 @@ private void GlobalOptionChanged(object? sender, OptionChangedEventArgs e)
/// <summary>
/// Callback from the OOP service back into us.
/// </summary>
public async ValueTask ReportTodoCommentDataAsync(DocumentId documentId, ImmutableArray<TodoCommentData> infos, CancellationToken cancellationToken)
public ValueTask ReportTodoCommentDataAsync(DocumentId documentId, ImmutableArray<TodoCommentData> infos, CancellationToken cancellationToken)
{
try
{
var workQueue = await _workQueueSource.Task.ConfigureAwait(false);
workQueue.AddWork(new DocumentAndComments(documentId, infos));
_workQueue.AddWork(new DocumentAndComments(documentId, infos));
return ValueTaskFactory.CompletedTask;
}
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, cancellationToken))
{
Expand Down
6 changes: 2 additions & 4 deletions src/VisualStudio/Core/Def/TableDataSource/AbstractTable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ protected AbstractTable(Workspace workspace, ITableManagerProvider provider, str
}

protected Workspace Workspace { get; }
internal ITableManager TableManager { get; }
internal abstract ImmutableArray<string> Columns { get; }

protected abstract void AddTableSourceIfNecessary(Solution solution);
protected abstract void RemoveTableSourceIfNecessary(Solution solution);
Expand Down Expand Up @@ -88,9 +90,5 @@ protected void AddInitialTableSource(Solution solution, ITableDataSource source)

protected void AddTableSource(ITableDataSource source)
=> this.TableManager.AddSource(source, Columns);

internal ITableManager TableManager { get; }

internal abstract ImmutableArray<string> Columns { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,18 +115,15 @@ protected override object GetOrUpdateAggregationKey(TodoItemsUpdatedArgs data)

private bool CheckAggregateKey(ImmutableArray<DocumentId> key, TodoItemsUpdatedArgs args)
{
if (args.DocumentId == null || args.Solution == null)
return true;

Contract.ThrowIfNull(args.Solution);
Contract.ThrowIfNull(args.DocumentId);
var documents = GetDocumentsWithSameFilePath(args.Solution, args.DocumentId);
return key == documents;
}

private object CreateAggregationKey(TodoItemsUpdatedArgs data)
{
if (data.Solution == null)
return GetItemKey(data);

Contract.ThrowIfNull(data.Solution);
return GetDocumentsWithSameFilePath(data.Solution, data.DocumentId);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ namespace Microsoft.VisualStudio.LanguageServices.Implementation.TodoComments
{
[Export(typeof(IVsTypeScriptTodoCommentService))]
[ExportEventListener(WellKnownEventListeners.Workspace, WorkspaceKind.Host), Shared]
internal class VisualStudioTodoCommentsService
: ForegroundThreadAffinitizedObject,
ITodoListProvider,
IVsTypeScriptTodoCommentService,
IEventListener<object>,
IDisposable
internal class VisualStudioTodoCommentsService :
ITodoListProvider,
IVsTypeScriptTodoCommentService,
IEventListener<object>,
IDisposable
{
private readonly IThreadingContext _threadingContext;
private readonly VisualStudioWorkspaceImpl _workspace;
private readonly EventListenerTracker<ITodoListProvider> _eventListenerTracker;
private readonly TodoCommentsListener _listener;
Expand All @@ -42,13 +42,13 @@ internal class VisualStudioTodoCommentsService
[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public VisualStudioTodoCommentsService(
IThreadingContext threadingContext,
VisualStudioWorkspaceImpl workspace,
IGlobalOptionService globalOptions,
IThreadingContext threadingContext,
IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider,
[ImportMany] IEnumerable<Lazy<IEventListener, EventListenerMetadata>> eventListeners)
: base(threadingContext)
{
_threadingContext = threadingContext;
_workspace = workspace;
_eventListenerTracker = new EventListenerTracker<ITodoListProvider>(eventListeners, WellKnownEventListeners.TodoListProvider);

Expand All @@ -59,12 +59,7 @@ public VisualStudioTodoCommentsService(
onTodoCommentsUpdated: (documentId, oldComments, newComments) =>
{
if (TodoListUpdated != null && !oldComments.SequenceEqual(newComments))
{
TodoListUpdated?.Invoke(
this, new TodoItemsUpdatedArgs(
documentId, _workspace, _workspace.CurrentSolution,
documentId.ProjectId, documentId, newComments));
}
TodoListUpdated?.Invoke(this, new TodoItemsUpdatedArgs(documentId, _workspace.CurrentSolution, documentId, newComments));
},
threadingContext.DisposalToken);
}
Expand All @@ -77,15 +72,20 @@ public void Dispose()
void IEventListener<object>.StartListening(Workspace workspace, object _)
{
if (workspace is VisualStudioWorkspace)
_ = StartAsync();
_ = StartAsync(workspace);
}

private async Task StartAsync()
private async Task StartAsync(Workspace workspace)
{
// Have to catch all exceptions coming through here as this is called from a
// fire-and-forget method and we want to make sure nothing leaks out.
try
{
// Don't bother doing anything until the workspace has actually loaded. We don't want to add to any
// startup costs by doing work too early.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we consider not doing anything at all until someone opens the todo comment window? I don't think I've ever opened it, so for people like me it seems like there is no point in this even running.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. taht woudl make a ton of sense. let me see what we can do about that.

var workspaceStatus = workspace.Services.GetRequiredService<IWorkspaceStatusService>();
await workspaceStatus.WaitUntilFullyLoadedAsync(_threadingContext.DisposalToken).ConfigureAwait(false);

// Now that we've started, let the VS todo list know to start listening to us
_eventListenerTracker.EnsureEventListener(_workspace, this);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -406,13 +406,13 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.Diagnostics

For Each group In map
RaiseEvent TodoListUpdated(Me, New TodoItemsUpdatedArgs(
Tuple.Create(Me, group.Key), workspace, workspace.CurrentSolution, group.Key.ProjectId, group.Key, group.ToImmutableArrayOrEmpty()))
Me, workspace.CurrentSolution, group.Key, group.ToImmutableArrayOrEmpty()))
Next
End Sub

Public Sub RaiseClearTodoListUpdated(workspace As Microsoft.CodeAnalysis.Workspace, documentId As DocumentId)
RaiseEvent TodoListUpdated(Me, New TodoItemsUpdatedArgs(
Tuple.Create(Me, documentId), workspace, workspace.CurrentSolution, documentId.ProjectId, documentId, ImmutableArray(Of TodoCommentData).Empty))
Me, workspace.CurrentSolution, documentId, ImmutableArray(Of TodoCommentData).Empty))
End Sub
End Class
End Class
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.SolutionCrawler;
using Microsoft.CodeAnalysis.TodoComments;
using Microsoft.ServiceHub.Framework;

namespace Microsoft.CodeAnalysis.Remote
{
Expand Down