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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Apr 10, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d86751e
Delay starting the work to scan for todo-items
CyrusNajmabadi Apr 8, 2022
260a72d
move properties
CyrusNajmabadi Apr 8, 2022
e502dd9
Cleanup
CyrusNajmabadi Apr 8, 2022
a0bb05a
Cleanup
CyrusNajmabadi Apr 8, 2022
afa7186
Fixup
CyrusNajmabadi Apr 8, 2022
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
1 change: 0 additions & 1 deletion src/Features/Core/Portable/TodoComments/AbstractTodoCommentsIncrementalAnalyzer.cs
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions .../InProcTodoCommentsIncrementalAnalyzer.cs → .../InProcTodoCommentsIncrementalAnalyzer.cs
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions ...odoCommentsIncrementalAnalyzerProvider.cs → ...odoCommentsIncrementalAnalyzerProvider.cs
View file
Open in desktop
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
24 changes: 13 additions & 11 deletions src/Features/LanguageServer/Protocol/Features/TodoComments/TodoCommentsListener.cs
View file
Open in desktop
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
25 changes: 15 additions & 10 deletions src/VisualStudio/Core/Def/TodoComments/VisualStudioTodoCommentsService.cs
View file
Open in desktop
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 Down Expand Up @@ -77,15 +77,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 workspae 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
2 changes: 0 additions & 2 deletions ...erviceHub/Services/TodoCommentsDiscovery/RemoteTodoCommentsIncrementalAnalyzerProvider.cs
View file
Open in desktop
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