Skip to content

Commit 8fefbab

Browse files
Remove most remaining workspace changed on UI thread (#79391)
This is a reapply of #78778 that got reverted due to a potential regression. Commit-at-a-time is recommended, since each use gets its own commit with further analysis there. The remaining uses of eventing that hit the UI thread (of any kind of workspace event) are: 1. Inline rename subscribes when there is an active rename to cancel on changes it doesn't know about. Since this is only active during the session, it doesn't seem worth it change it. 2. MiscellanousFilesWorkspace subscribes to workspace _registration_ changed, which is when a document is opened/closed, and that's not urgent to fix and restricted to just registration events. 3. XamlProjectService subscribes to document _closed_. This only happens if XAML is loaded in the first place, and then is only raised on the close path. I looked at making a small change to move it off but the code is generally quite scary and since it's document close only that's not chatty. The last commit makes a larger change for performance to avoid a regression in our internal performance tests. To explain the change a bit of explanation is needed: Our WorkspaceChanged events are raised with an batching work queue with a delay of zero. When we have events to raise on the UI thread, the batch handler first raises the background thread notifications, then jumps to the UI thread (if needed) and raises them there. This means that in this case, our events are roughly 'throttled' by the availability of the UI thread; so we're likely to end up with a flurry of background events, then foreground events, then background events, etc. Usually these event handlers are feeding into other batching work queues, including the one we use to synchronize the VS Workspace to our OOP process to keep things relatively up to date. That had a delay of 50ms to batch up a request to start syncing it again. My belief is that prior to the WorkspaceChanged change, that delay didn't matter so much -- the actual batching may have been that if (say) the UI thread was only available once every 100ms to fire events, then that was the real rate limiting. Once that is freed up, then now we were creating a lot more batches. I did some tests counting the number of batches during a Roslyn solution load and it seemed to be roughly double, which matched some of the extra allocation overhead happening in StreamJsonRpc as we were synchronizing with OOP. I have changed the batching delay for the OOP sync from 50ms to 250ms, that number chosen because that's what "Short" is in our delay choices. The excess memory allocations seen before have disappeared, making me think we're now doing fewer but larger batches. The ManagedLanguages.SolutionManagement RPS test shows a 10% reduction in ServiceHub allocations than the baseline. I don't imagine this would be particularly noticeable to a user the extra delay since that'd only impact the final sync that happens after a solution load, which isn't hugely critical. The immediate-sync that we have of a changed document is left in place.
2 parents e8bbb65 + b17682a commit 8fefbab

File tree

8 files changed

+20
-33
lines changed

8 files changed

+20
-33
lines changed

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,13 @@ public SolutionChecksumUpdater(
5858

5959
_shutdownToken = shutdownToken;
6060

61+
// A time span of Short is chosen here to ensure that the batching favors fewer but larger batches.
62+
// During solution load a large number of WorkspaceChange events might be raised over a few seconds,
63+
// and in performance tests a 50ms delay was found to be causing a lot of extra memory churn synchronizing
64+
// things OOP. Short didn't cause a similar issue; it's possible this will need to be fine tuned for something in
65+
// the middle.
6166
_synchronizeWorkspaceQueue = new AsyncBatchingWorkQueue(
62-
DelayTimeSpan.NearImmediate,
67+
DelayTimeSpan.Short,
6368
SynchronizePrimaryWorkspaceAsync,
6469
listener,
6570
shutdownToken);

src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,8 @@ private sealed class DocumentActiveContextChangedEventSource(ITextBuffer subject
1313
{
1414
private WorkspaceEventRegistration? _documentActiveContextChangedDisposer;
1515

16-
// Require main thread on the callback as RaiseChanged implementors may have main thread dependencies.
1716
protected override void ConnectToWorkspace(Workspace workspace)
18-
=> _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
17+
=> _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged);
1918

2019
protected override void DisconnectFromWorkspace(Workspace workspace)
2120
=> _documentActiveContextChangedDisposer?.Dispose();

src/EditorFeatures/Core/Tagging/ITaggerEventSource.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ internal interface ITaggerEventSource
4141

4242
/// <summary>
4343
/// An event has happened on the thing the tagger is attached to. The tagger should
44-
/// recompute tags.
44+
/// recompute tags. May be raised on any thread.
4545
/// </summary>
4646
event EventHandler<TaggerEventArgs> Changed;
4747
}

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,7 @@ public CodeAnalysisDiagnosticAnalyzerService(
4949
_workspace = workspace;
5050
_diagnosticAnalyzerService = _workspace.Services.GetRequiredService<IDiagnosticAnalyzerService>();
5151

52-
// Main thread as OnWorkspaceChanged's call to IDiagnosticAnalyzerService.RequestDiagnosticRefresh isn't clear on
53-
// threading requirements
54-
_ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
52+
_ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged);
5553
}
5654

5755
private void OnWorkspaceChanged(WorkspaceChangeEventArgs e)

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ internal interface IDiagnosticAnalyzerService : IWorkspaceService
2222
/// <summary>
2323
/// Re-analyze all projects and documents. This will cause an LSP diagnostic refresh request to be sent.
2424
/// </summary>
25+
/// <remarks>
26+
/// This implementation must be safe to call on any thread.
27+
/// </remarks>
2528
void RequestDiagnosticRefresh();
2629

2730
/// <summary>

src/LanguageServer/Protocol/Workspaces/LspWorkspaceRegistrationService.cs

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,7 @@ public virtual void Register(Workspace? workspace)
3939
m["WorkspacePartialSemanticsEnabled"] = workspace.PartialSemanticsEnabled;
4040
}, workspace));
4141

42-
// Forward workspace change events for all registered LSP workspaces. Requires main thread as it
43-
// fires LspSolutionChanged which hasn't been guaranteed to be thread safe.
44-
var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
42+
var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged);
4543

4644
lock (_gate)
4745
{
@@ -94,9 +92,7 @@ public void Dispose()
9492
}
9593

9694
/// <summary>
97-
/// Indicates whether the LSP solution has changed in a non-tracked document context.
98-
///
99-
/// <b>IMPORTANT:</b> Implementations of this event handler should do as little synchronous work as possible since this will block.
95+
/// Indicates whether the LSP solution has changed in a non-tracked document context. May be raised on any thread.
10096
/// </summary>
10197
public EventHandler<WorkspaceChangeEventArgs>? LspSolutionChanged;
10298
}

src/VisualStudio/Core/Def/StackTraceExplorer/StackTraceExplorerViewModel.cs

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,6 @@ public StackTraceExplorerViewModel(IThreadingContext threadingContext, Workspace
5050
_threadingContext = threadingContext;
5151
_workspace = workspace;
5252

53-
// Main thread dependency as Workspace_WorkspaceChanged modifies an ObservableCollection
54-
_ = workspace.RegisterWorkspaceChangedHandler(Workspace_WorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
55-
5653
_classificationTypeMap = classificationTypeMap;
5754
_formatMap = formatMap;
5855

@@ -103,15 +100,6 @@ private void CallstackLines_CollectionChanged(object sender, System.Collections.
103100
NotifyPropertyChanged(nameof(IsInstructionTextVisible));
104101
}
105102

106-
private void Workspace_WorkspaceChanged(WorkspaceChangeEventArgs e)
107-
{
108-
if (e.Kind == WorkspaceChangeKind.SolutionChanged)
109-
{
110-
Selection = null;
111-
Frames.Clear();
112-
}
113-
}
114-
115103
private FrameViewModel GetViewModel(ParsedFrame frame)
116104
=> frame switch
117105
{

src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ internal sealed partial class CpsDiagnosticItemSource : BaseDiagnosticAndGenerat
1919
{
2020
private readonly IVsHierarchyItem _item;
2121
private readonly string _projectDirectoryPath;
22+
private readonly string? _analyzerFilePath;
2223

2324
private WorkspaceEventRegistration? _workspaceChangedDisposer;
2425

@@ -37,6 +38,8 @@ public CpsDiagnosticItemSource(
3738
_item = item;
3839
_projectDirectoryPath = Path.GetDirectoryName(projectPath);
3940

41+
_analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, _item.CanonicalName);
42+
4043
this.AnalyzerReference = TryGetAnalyzerReference(Workspace.CurrentSolution);
4144
if (this.AnalyzerReference == null)
4245
{
@@ -47,9 +50,7 @@ public CpsDiagnosticItemSource(
4750
// then connect to it.
4851
if (workspace.CurrentSolution.ContainsProject(projectId))
4952
{
50-
// Main thread dependency as OnWorkspaceChangedLookForAnalyzer accesses the IVsHierarchy
51-
// and fires the PropertyChanged event
52-
_workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer, WorkspaceEventOptions.RequiresMainThreadOptions);
53+
_workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer);
5354
item.PropertyChanged += IVsHierarchyItem_PropertyChanged;
5455

5556
// Now that we've subscribed, check once more in case we missed the event
@@ -118,14 +119,11 @@ private void OnWorkspaceChangedLookForAnalyzer(WorkspaceChangeEventArgs e)
118119
return null;
119120
}
120121

121-
var canonicalName = _item.CanonicalName;
122-
var analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, canonicalName);
123-
124-
if (string.IsNullOrEmpty(analyzerFilePath))
122+
if (string.IsNullOrEmpty(_analyzerFilePath))
125123
{
126124
return null;
127125
}
128126

129-
return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, analyzerFilePath, StringComparison.OrdinalIgnoreCase));
127+
return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, _analyzerFilePath, StringComparison.OrdinalIgnoreCase));
130128
}
131129
}

0 commit comments

Comments
 (0)