Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c553551
WIP: Start moving workspace events to fire on background threads
ToddGrun Apr 13, 2025
71b1d43
1) Move Dispose methods closer to construction
ToddGrun Apr 13, 2025
c2e874f
Remove the asynchronous eventing and switch back to a synchronous mod…
ToddGrun Apr 14, 2025
cdceae8
Implement missed workspace event (WorkspaceFailed)
ToddGrun Apr 14, 2025
b0a49f8
mostly formatting cleanup
ToddGrun Apr 14, 2025
b1f1b52
Move a couple event handlers back to the main thread that appear to n…
ToddGrun Apr 14, 2025
c48694a
A couple more small cleanups
ToddGrun Apr 14, 2025
be3fd62
Move EventMap/WorkspaceEventOptions out of SharedUtilitiesAndExtensions
ToddGrun Apr 15, 2025
2254c05
use WorkspaceEventRegistration type directly instead of IDisposable
ToddGrun Apr 15, 2025
eaec791
PR feedback
ToddGrun Apr 15, 2025
7ad41df
Create enum WorkspaceEventType for strings representing workspace eve…
ToddGrun Apr 15, 2025
7c21263
1) move a couple declarations around
ToddGrun Apr 15, 2025
dbc299e
1) Comment and add tuple name to _disposableEventHandlers
ToddGrun Apr 15, 2025
76229c7
Better handle getting advised by the same event handler multiple times
ToddGrun Apr 16, 2025
92876f7
Fix an issue found by lsp unit tests where the Register/Deregister ca…
ToddGrun Apr 16, 2025
d0496b2
Merge branch 'main' into dev/toddgrun/WorkspaceEventsToBackgroundThreads
ToddGrun Apr 18, 2025
45c094f
Merge branch 'main' into dev/toddgrun/WorkspaceEventsToBackgroundThreads
ToddGrun Apr 24, 2025
34dd6ed
1) Null out a couple members in SyntacticClassificationTaggerProvider
ToddGrun Apr 24, 2025
5c66609
Comment cleanup, remove improper assert
ToddGrun Apr 26, 2025
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
23 changes: 19 additions & 4 deletions src/Compilers/Test/Core/FX/EventWaiter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,7 @@
#nullable disable

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace Roslyn.Test.Utilities
{
Expand Down Expand Up @@ -55,6 +51,25 @@ public EventHandler<T> Wrap<T>(EventHandler<T> input)
};
}

public Action<TEventArgs> Wrap<TEventArgs>(Action<TEventArgs> input)
{
return (args) =>
{
try
{
input(args);
}
catch (Exception ex)
{
_capturedException = ex;
}
finally
{
_eventSignal.Set();
}
};
}

/// <summary>
/// Use this method to block the test until the operation enclosed in the Wrap method completes
/// </summary>
Expand Down
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea why this file is here rather than the Workspaces layer. Or minimally, why it's in the C# folder...

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ private sealed record CachedServices(
private readonly SyntacticClassificationTaggerProvider _taggerProvider;
private readonly ITextBuffer2 _subjectBuffer;
private readonly WorkspaceRegistration _workspaceRegistration;
private WorkspaceEventRegistration? _workspaceChangedDisposer;
private WorkspaceEventRegistration? _workspaceDocumentActiveContextChangedDisposer;

private readonly CancellationTokenSource _disposalCancellationSource = new();

Expand Down Expand Up @@ -224,8 +226,8 @@ private void ConnectToWorkspace(Workspace workspace)
_taggerProvider.ThreadingContext.ThrowIfNotOnUIThread();

_workspace = workspace;
_workspace.WorkspaceChanged += this.OnWorkspaceChanged;
_workspace.DocumentActiveContextChanged += this.OnDocumentActiveContextChanged;
_workspaceChangedDisposer = _workspace.RegisterWorkspaceChangedHandler(this.OnWorkspaceChanged);
_workspaceDocumentActiveContextChangedDisposer = _workspace.RegisterDocumentActiveContextChangedHandler(this.OnDocumentActiveContextChanged);

// Now that we've connected to the workspace, kick off work to reclassify this buffer.
_workQueue.AddWork(_subjectBuffer.CurrentSnapshot);
Expand All @@ -243,8 +245,8 @@ public void DisconnectFromWorkspace()

if (_workspace != null)
{
_workspace.WorkspaceChanged -= this.OnWorkspaceChanged;
_workspace.DocumentActiveContextChanged -= this.OnDocumentActiveContextChanged;
_workspaceChangedDisposer?.Dispose();
_workspaceDocumentActiveContextChangedDisposer?.Dispose();

_workspace = null;

Expand All @@ -264,7 +266,7 @@ private void OnSubjectBufferChanged(object? sender, TextContentChangedEventArgs
_workQueue.AddWork(args.After);
}

private void OnDocumentActiveContextChanged(object? sender, DocumentActiveContextChangedEventArgs args)
private void OnDocumentActiveContextChanged(DocumentActiveContextChangedEventArgs args)
{
if (_workspace == null)
return;
Expand All @@ -277,18 +279,19 @@ private void OnDocumentActiveContextChanged(object? sender, DocumentActiveContex
_workQueue.AddWork(_subjectBuffer.CurrentSnapshot);
}

private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs args)
private void OnWorkspaceChanged(WorkspaceChangeEventArgs args)
{
// We may be getting an event for a workspace we already disconnected from. If so,
// ignore them. We won't be able to find the Document corresponding to our text buffer,
// so we can't reasonably classify this anyways.
if (args.NewSolution.Workspace != _workspace)
var workspace = _workspace;
if (args.NewSolution.Workspace != workspace)
return;

if (args.Kind != WorkspaceChangeKind.ProjectChanged)
return;

var documentId = _workspace.GetDocumentIdInCurrentContext(_subjectBuffer.AsTextContainer());
var documentId = workspace.GetDocumentIdInCurrentContext(_subjectBuffer.AsTextContainer());
if (args.ProjectId != documentId?.ProjectId)
return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.Text.Shared.Extensions;
using Microsoft.VisualStudio.Text;
Expand Down Expand Up @@ -103,6 +103,8 @@ internal sealed class TrackingSession
private readonly CancellationTokenSource _cancellationSource = new();
private readonly IActiveStatementSpanFactory _spanProvider;
private readonly ICompileTimeSolutionProvider _compileTimeSolutionProvider;
private readonly WorkspaceEventRegistration _documentOpenedHandlerDisposer;
private readonly WorkspaceEventRegistration _documentClosedHandlerDisposer;

#region lock(_trackingSpans)

Expand All @@ -121,8 +123,8 @@ public TrackingSession(Workspace workspace, IActiveStatementSpanFactory spanProv
_spanProvider = spanProvider;
_compileTimeSolutionProvider = workspace.Services.GetRequiredService<ICompileTimeSolutionProvider>();

_workspace.DocumentOpened += DocumentOpened;
_workspace.DocumentClosed += DocumentClosed;
_documentOpenedHandlerDisposer = _workspace.RegisterDocumentOpenedHandler(DocumentOpened);
_documentClosedHandlerDisposer = _workspace.RegisterDocumentClosedHandler(DocumentClosed);
}

internal Dictionary<string, ImmutableArray<ActiveStatementTrackingSpan>> Test_GetTrackingSpans()
Expand All @@ -133,16 +135,16 @@ public void EndTracking()
_cancellationSource.Cancel();
_cancellationSource.Dispose();

_workspace.DocumentOpened -= DocumentOpened;
_workspace.DocumentClosed -= DocumentClosed;
_documentOpenedHandlerDisposer.Dispose();
_documentClosedHandlerDisposer.Dispose();

lock (_trackingSpans)
{
_trackingSpans.Clear();
}
}

private void DocumentClosed(object? sender, DocumentEventArgs e)
private void DocumentClosed(DocumentEventArgs e)
{
if (e.Document.FilePath != null)
{
Expand All @@ -153,7 +155,7 @@ private void DocumentClosed(object? sender, DocumentEventArgs e)
}
}

private void DocumentOpened(object? sender, DocumentEventArgs e)
private void DocumentOpened(DocumentEventArgs e)
=> _ = TrackActiveSpansAsync(e.Document);

private async Task TrackActiveSpansAsync(Document designTimeDocument)
Expand Down
14 changes: 8 additions & 6 deletions src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ internal sealed class SolutionChecksumUpdater

private readonly object _gate = new();
private bool _isSynchronizeWorkspacePaused;
private readonly WorkspaceEventRegistration _workspaceChangedDisposer;
private readonly WorkspaceEventRegistration _workspaceChangedImmediateDisposer;

private readonly CancellationToken _shutdownToken;

Expand Down Expand Up @@ -79,8 +81,8 @@ public SolutionChecksumUpdater(
shutdownToken);

// start listening workspace change event
_workspace.WorkspaceChanged += OnWorkspaceChanged;
_workspace.WorkspaceChangedImmediate += OnWorkspaceChangedImmediate;
_workspaceChangedDisposer = _workspace.RegisterWorkspaceChangedHandler(this.OnWorkspaceChanged);
_workspaceChangedImmediateDisposer = _workspace.RegisterWorkspaceChangedImmediateHandler(OnWorkspaceChangedImmediate);
_documentTrackingService.ActiveDocumentChanged += OnActiveDocumentChanged;

if (_globalOperationService != null)
Expand All @@ -100,8 +102,8 @@ public void Shutdown()
PauseSynchronizingPrimaryWorkspace();

_documentTrackingService.ActiveDocumentChanged -= OnActiveDocumentChanged;
_workspace.WorkspaceChanged -= OnWorkspaceChanged;
_workspace.WorkspaceChangedImmediate -= OnWorkspaceChangedImmediate;
_workspaceChangedDisposer.Dispose();
_workspaceChangedImmediateDisposer.Dispose();

if (_globalOperationService != null)
{
Expand Down Expand Up @@ -136,7 +138,7 @@ private void ResumeSynchronizingPrimaryWorkspace()
}
}

private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e)
private void OnWorkspaceChanged(WorkspaceChangeEventArgs _)
{
// Check if we're currently paused. If so ignore this notification. We don't want to any work in response
// to whatever the workspace is doing.
Expand All @@ -147,7 +149,7 @@ private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e)
}
}

private void OnWorkspaceChangedImmediate(object? sender, WorkspaceChangeEventArgs e)
private void OnWorkspaceChangedImmediate(WorkspaceChangeEventArgs e)
{
if (e.Kind == WorkspaceChangeKind.DocumentChanged)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,21 @@ internal partial class TaggerEventSources
{
private sealed class DocumentActiveContextChangedEventSource(ITextBuffer subjectBuffer) : AbstractWorkspaceTrackingTaggerEventSource(subjectBuffer)
{
private WorkspaceEventRegistration? _documentActiveContextChangedDisposer;

// Require main thread on the callback as RaiseChanged implementors may have main thread dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

Was this a generic concern about risk, or do we actually have a known case where we still have a dependency? What's surprising here to me is we have a bunch of event sources, not just this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't do the transitive look into whether advisers to AbstractTaggerEventSource.Changed have main thread dependencies.

Generally, I wanted to be conservative in the callbacks moved off the main thread in this PR. I plan to have another PR after this one that will finish off the workspace event switch to the new API and at that point, I'll reprofile and see if any of the remaining main thread callbacks are showing up in the profile.

protected override void ConnectToWorkspace(Workspace workspace)
=> workspace.DocumentActiveContextChanged += OnDocumentActiveContextChanged;
=> _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged, WorkspaceEventOptions.RequiresMainThreadOptions);

protected override void DisconnectFromWorkspace(Workspace workspace)
=> workspace.DocumentActiveContextChanged -= OnDocumentActiveContextChanged;
=> _documentActiveContextChangedDisposer?.Dispose();

private void OnDocumentActiveContextChanged(object? sender, DocumentActiveContextChangedEventArgs e)
private void OnDocumentActiveContextChanged(DocumentActiveContextChangedEventArgs e)
{
var document = SubjectBuffer.AsTextContainer().GetOpenDocumentInCurrentContext();

if (document != null && document.Id == e.NewActiveContextDocumentId)
{
this.RaiseChanged();
}
}
}
}
2 changes: 1 addition & 1 deletion src/Features/Lsif/Generator/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ private static async Task GenerateWithMSBuildWorkspaceAsync(
var solutionLoadStopwatch = Stopwatch.StartNew();

using var msbuildWorkspace = MSBuildWorkspace.Create(await Composition.CreateHostServicesAsync());
msbuildWorkspace.WorkspaceFailed += (s, e) => logger.Log(e.Diagnostic.Kind == WorkspaceDiagnosticKind.Failure ? LogLevel.Error : LogLevel.Warning, "Problem while loading: " + e.Diagnostic.Message);
_ = msbuildWorkspace.RegisterWorkspaceFailedHandler((e) => logger.Log(e.Diagnostic.Kind == WorkspaceDiagnosticKind.Failure ? LogLevel.Error : LogLevel.Warning, "Problem while loading: " + e.Diagnostic.Message));

var solution = await openAsync(msbuildWorkspace, cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,8 @@ private DiagnosticIncrementalAnalyzer CreateIncrementalAnalyzer(Workspace worksp
private DiagnosticIncrementalAnalyzer CreateIncrementalAnalyzerCallback(Workspace workspace)
{
// subscribe to active context changed event for new workspace
workspace.DocumentActiveContextChanged += OnDocumentActiveContextChanged;
_ = workspace.RegisterDocumentActiveContextChangedHandler(args => RequestDiagnosticRefresh());

return new DiagnosticIncrementalAnalyzer(this, AnalyzerInfoCache, this.GlobalOptions);
}

private void OnDocumentActiveContextChanged(object? sender, DocumentActiveContextChangedEventArgs e)
=> RequestDiagnosticRefresh();
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ namespace Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService
/// Creates services on the first connection of an applicable subject buffer to an IWpfTextView.
/// This ensures the services are available by the time an open document or the interactive window needs them.
/// </summary>
internal abstract class AbstractCreateServicesOnTextViewConnection : IWpfTextViewConnectionListener
internal abstract class AbstractCreateServicesOnTextViewConnection : IWpfTextViewConnectionListener, IDisposable
{
private readonly string _languageName;
private readonly AsyncBatchingWorkQueue<ProjectId?> _workQueue;
private readonly WorkspaceEventRegistration _workspaceDocumentOpenedDisposer;
private bool _initialized = false;

protected VisualStudioWorkspace Workspace { get; }
Expand All @@ -56,9 +57,12 @@ public AbstractCreateServicesOnTextViewConnection(
listenerProvider.GetListener(FeatureAttribute.CompletionSet),
threadingContext.DisposalToken);

Workspace.DocumentOpened += QueueWorkOnDocumentOpened;
_workspaceDocumentOpenedDisposer = Workspace.RegisterDocumentOpenedHandler(QueueWorkOnDocumentOpened);
Copy link
Member

Choose a reason for hiding this comment

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

You have correctly maintained this code's behavior, but as we're continuing to push on load performance, this is really not ideal that we're forcing the VisualStudioWorkspace to be created when a C# file is opened. If all you're doing is opening a loose file...that was extra work for no real benefit, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. If you have the chance, can you log a bug and I'll take a look as a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

Filed #78355

}

public void Dispose()
=> _workspaceDocumentOpenedDisposer.Dispose();

void IWpfTextViewConnectionListener.SubjectBuffersConnected(IWpfTextView textView, ConnectionReason reason, Collection<ITextBuffer> subjectBuffers)
{
if (!_initialized)
Expand Down Expand Up @@ -96,7 +100,7 @@ private async ValueTask BatchProcessProjectsWithOpenedDocumentAsync(ImmutableSeg
}
}

private void QueueWorkOnDocumentOpened(object sender, DocumentEventArgs e)
private void QueueWorkOnDocumentOpened(DocumentEventArgs e)
{
if (e.Document.Project.Language == _languageName)
_workQueue.AddWork(e.Document.Project.Id);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.TextManager.Interop;
using Roslyn.Utilities;
using static Microsoft.CodeAnalysis.WorkspaceEventMap;

namespace Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem;

Expand Down Expand Up @@ -172,7 +173,11 @@ private void Registration_WorkspaceChanged(object sender, EventArgs e)
// to the RDT in the background thread. Since this is all asynchronous a bit more asynchrony is fine.
if (!_threadingContext.JoinableTaskContext.IsOnMainThread)
{
ScheduleTask(() => Registration_WorkspaceChanged(sender, e));
// Require main thread on the callback as this method requires that per the comment above.
var handlerAndOptions = new WorkspaceEventHandlerAndOptions(args => Registration_WorkspaceChanged(sender, e), WorkspaceEventOptions.RequiresMainThreadOptions);
var handlerSet = EventHandlerSet.Create(handlerAndOptions);

ScheduleTask(e, handlerSet);
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,9 @@ public static async Task CodeActionAsync(
CancellationToken cancellationToken = default)
{
var events = new List<WorkspaceChangeEventArgs>();
void WorkspaceChangedHandler(object sender, WorkspaceChangeEventArgs e) => events.Add(e);

var workspace = await textViewWindowVerifier.TestServices.Shell.GetComponentModelServiceAsync<VisualStudioWorkspace>(cancellationToken);
using var workspaceEventRestorer = WithWorkspaceChangedHandler(workspace, WorkspaceChangedHandler);
using var _ = workspace.RegisterWorkspaceChangedHandler(e => events.Add(e));

await textViewWindowVerifier.TestServices.Editor.ShowLightBulbAsync(cancellationToken);

Expand Down Expand Up @@ -155,12 +154,6 @@ await textViewWindowVerifier.TestServices.Workspace.WaitForAllAsyncOperationsAsy
Assert.NotEqual("text", tokenType);
}

private static WorkspaceEventRestorer WithWorkspaceChangedHandler(Workspace workspace, EventHandler<WorkspaceChangeEventArgs> eventHandler)
{
workspace.WorkspaceChanged += eventHandler;
return new WorkspaceEventRestorer(workspace, eventHandler);
}

private static LoggerRestorer WithLogger(ILogger logger)
{
return new LoggerRestorer(Logger.SetLogger(logger));
Expand Down Expand Up @@ -195,23 +188,6 @@ public void LogBlockStart(FunctionId functionId, LogMessage logMessage, int uniq
}
}

private readonly struct WorkspaceEventRestorer : IDisposable
{
private readonly Workspace _workspace;
private readonly EventHandler<WorkspaceChangeEventArgs> _eventHandler;

public WorkspaceEventRestorer(Workspace workspace, EventHandler<WorkspaceChangeEventArgs> eventHandler)
{
_workspace = workspace;
_eventHandler = eventHandler;
}

public void Dispose()
{
_workspace.WorkspaceChanged -= _eventHandler;
}
}

private readonly struct LoggerRestorer : IDisposable
{
private readonly ILogger? _logger;
Expand Down
Loading