From e0765de8d72994405b52b1b255f52c7c3ce973dc Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Sun, 26 Jan 2025 08:12:44 -0800 Subject: [PATCH 01/14] WIP: Add new workspace event that gives handlers the opportunity to be processed immediately This is in response to allocations seen when the OOP process attempts to operate on a sourcetext version that has not been synchronized over. In those cases, the OOP process requests a full deserialization of the source text causing full buffer allocations in both VS and the CA process. There are several points of asynchronicity in the current system around sending over buffer changes, reducing any of which would reduce the likelihood of needing this full serialization. This PR removes one of those points of asynchronicity, specifically in the workspace eventing layer. Previously, all notiications were done on a delayed basis, by wrapping each notification in the WorkspaceSpace.ScheduleTask call from the RaiseWorkspaceChangedEventAsync method. This method allows callers to indicate they need to be called immediately upon the workspace change. As noted by a doc comment in the PR, these handlers should be very fast. Going to mark this as draft and get speedometer numbers off this to see if this helps enough, or if instead the other points of asynchronicity should be investigated (specifically, the usage of the _textChangeQueue ABWQ). There are other alternatives within the text syncing system such as allowing OOP to request a text change instead of the full buffer contents, but previous investigations into that ended up complicated and incomplete. --- .../Core/Remote/SolutionChecksumUpdater.cs | 21 +++++++----- .../Portable/Workspace/Workspace_Events.cs | 33 +++++++++++++++++-- .../Compiler/Core/Log/FunctionId.cs | 2 ++ 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index 6f1b14d5dc446..3b29f4361670e 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -87,6 +87,7 @@ public SolutionChecksumUpdater( // start listening workspace change event _workspace.WorkspaceChanged += OnWorkspaceChanged; + _workspace.WorkspaceChangedImmediate += OnWorkspaceChangedImmediate; _documentTrackingService.ActiveDocumentChanged += OnActiveDocumentChanged; if (_globalOperationService != null) @@ -107,6 +108,7 @@ public void Shutdown() _documentTrackingService.ActiveDocumentChanged -= OnActiveDocumentChanged; _workspace.WorkspaceChanged -= OnWorkspaceChanged; + _workspace.WorkspaceChangedImmediate -= OnWorkspaceChangedImmediate; if (_globalOperationService != null) { @@ -143,14 +145,6 @@ private void ResumeSynchronizingPrimaryWorkspace() private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e) { - if (e.Kind == WorkspaceChangeKind.DocumentChanged) - { - var oldDocument = e.OldSolution.GetDocument(e.DocumentId); - var newDocument = e.NewSolution.GetDocument(e.DocumentId); - if (oldDocument != null && newDocument != null) - _textChangeQueue.AddWork((oldDocument, newDocument)); - } - // 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. lock (_gate) @@ -160,6 +154,17 @@ private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e) } } + private void OnWorkspaceChangedImmediate(object? sender, WorkspaceChangeEventArgs e) + { + if (e.Kind == WorkspaceChangeKind.DocumentChanged) + { + var oldDocument = e.OldSolution.GetDocument(e.DocumentId); + var newDocument = e.NewSolution.GetDocument(e.DocumentId); + if (oldDocument != null && newDocument != null) + _textChangeQueue.AddWork((oldDocument, newDocument)); + } + } + private void OnActiveDocumentChanged(object? sender, DocumentId? e) => _synchronizeActiveDocumentQueue.AddWork(); diff --git a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs index a54a64ccfc8bf..6d6f39e54eea9 100644 --- a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs +++ b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs @@ -19,6 +19,7 @@ public abstract partial class Workspace private readonly EventMap _eventMap = new(); private const string WorkspaceChangeEventName = "WorkspaceChanged"; + private const string WorkspaceChangedImmediateEventName = "WorkspaceChangedImmediate"; private const string WorkspaceFailedEventName = "WorkspaceFailed"; private const string DocumentOpenedEventName = "DocumentOpened"; private const string DocumentClosedEventName = "DocumentClosed"; @@ -42,6 +43,23 @@ public event EventHandler WorkspaceChanged } } + /// + /// An event raised *immediately* whenever the current solution is changed. Handlers + /// should be written to be very fast. + /// + internal event EventHandler WorkspaceChangedImmediate + { + add + { + _eventMap.AddEventHandler(WorkspaceChangedImmediateEventName, value); + } + + remove + { + _eventMap.RemoveEventHandler(WorkspaceChangedImmediateEventName, value); + } + } + protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solution oldSolution, Solution newSolution, ProjectId projectId = null, DocumentId documentId = null) { if (newSolution == null) @@ -59,14 +77,25 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio projectId = documentId.ProjectId; } - var ev = GetEventHandlers(WorkspaceChangeEventName); + var ev = GetEventHandlers(WorkspaceChangedImmediateEventName); + WorkspaceChangeEventArgs? args = null; + if (ev.HasHandlers) + { + using (Logger.LogBlock(FunctionId.Workspace_EventsImmediate, (s, p, d, k) => $"{s.Id} - {p} - {d} {kind.ToString()}", newSolution, projectId, documentId, kind, CancellationToken.None)) + { + args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); + ev.RaiseEvent(static (handler, arg) => handler(arg.self, arg.args), (self: this, args)); + } + } + + ev = GetEventHandlers(WorkspaceChangeEventName); if (ev.HasHandlers) { return this.ScheduleTask(() => { using (Logger.LogBlock(FunctionId.Workspace_Events, (s, p, d, k) => $"{s.Id} - {p} - {d} {kind.ToString()}", newSolution, projectId, documentId, kind, CancellationToken.None)) { - var args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); + args ??= new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); ev.RaiseEvent(static (handler, arg) => handler(arg.self, arg.args), (self: this, args)); } }, WorkspaceChangeEventName); diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs index 4f123aa508e83..085a5982b405a 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs @@ -484,6 +484,8 @@ internal enum FunctionId Renamer_FindRenameLocationsAsync = 387, Renamer_ResolveConflictsAsync = 388, + Workspace_EventsImmediate = 389, + ChangeSignature_Data = 400, AbstractEncapsulateFieldService_EncapsulateFieldsAsync = 410, From 0394a814756ffa745c7aaa90e34987fbc1774a66 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Sun, 26 Jan 2025 08:30:43 -0800 Subject: [PATCH 02/14] don't use nullability annotations --- src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs index 6d6f39e54eea9..9ca563c476893 100644 --- a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs +++ b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs @@ -78,7 +78,7 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio } var ev = GetEventHandlers(WorkspaceChangedImmediateEventName); - WorkspaceChangeEventArgs? args = null; + WorkspaceChangeEventArgs args = null; if (ev.HasHandlers) { using (Logger.LogBlock(FunctionId.Workspace_EventsImmediate, (s, p, d, k) => $"{s.Id} - {p} - {d} {kind.ToString()}", newSolution, projectId, documentId, kind, CancellationToken.None)) From 445cc6c2f95c5e518b94600e3b71dbe133c36412 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Sun, 26 Jan 2025 20:04:35 -0800 Subject: [PATCH 03/14] Add a big old range for funciton ids that don't fit into other categories --- .../Compiler/Core/Log/FunctionId.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs index 085a5982b405a..7dcd74b4ca00f 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs @@ -484,8 +484,6 @@ internal enum FunctionId Renamer_FindRenameLocationsAsync = 387, Renamer_ResolveConflictsAsync = 388, - Workspace_EventsImmediate = 389, - ChangeSignature_Data = 400, AbstractEncapsulateFieldService_EncapsulateFieldsAsync = 410, @@ -641,4 +639,7 @@ internal enum FunctionId VSCode_LanguageServer_Started = 860, VSCode_Project_Load_Started = 861, VSCode_Projects_Load_Completed = 862, + + // 900-999 for items that don't fit into other categories. + Workspace_EventsImmediate = 900, } From b1797c27933aea5c627598c9b4b40fb1ba91bb0d Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Mon, 27 Jan 2025 06:39:48 -0800 Subject: [PATCH 04/14] Remove the _textChange ABWQ point of asynchronicity to try and further reduce the amount of serialization and deserialization --- .../Core/Remote/SolutionChecksumUpdater.cs | 91 ++++++++----------- ...isualStudioWorkspaceServiceHubConnector.cs | 6 +- .../RemoteHostClientServiceFactoryTests.cs | 4 +- .../Services/ServiceHubServicesTests.cs | 2 +- .../Services/SolutionServiceTests.cs | 7 +- .../IRemoteAssetSynchronizationService.cs | 5 +- .../RemoteAssetSynchronizationService.cs | 35 ++++--- 7 files changed, 71 insertions(+), 79 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index 3b29f4361670e..f7298200f775b 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -3,16 +3,13 @@ // See the LICENSE file in the project root for more information. using System; -using System.Collections.Immutable; using System.Linq; using System.Threading; using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Collections; +using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Internal.Log; using Microsoft.CodeAnalysis.Notification; -using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.TestHooks; -using Microsoft.CodeAnalysis.Text; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.Remote; @@ -33,13 +30,6 @@ internal sealed class SolutionChecksumUpdater private readonly IDocumentTrackingService _documentTrackingService; - /// - /// Queue to push out text changes in a batched fashion when we hear about them. Because these should be short - /// operations (only syncing text changes) we don't cancel this when we enter the paused state. We simply don't - /// start queuing more requests into this until we become unpaused. - /// - private readonly AsyncBatchingWorkQueue<(Document oldDocument, Document newDocument)> _textChangeQueue; - /// /// Queue for kicking off the work to synchronize the primary workspace's solution. /// @@ -53,9 +43,13 @@ internal sealed class SolutionChecksumUpdater private readonly object _gate = new(); private bool _isSynchronizeWorkspacePaused; + private readonly IThreadingContext _threadingContext; + private readonly CancellationToken _shutdownToken; + public SolutionChecksumUpdater( Workspace workspace, IAsynchronousOperationListenerProvider listenerProvider, + IThreadingContext threadingContext, CancellationToken shutdownToken) { var listener = listenerProvider.GetListener(FeatureAttribute.SolutionChecksumUpdater); @@ -65,20 +59,15 @@ public SolutionChecksumUpdater( _workspace = workspace; _documentTrackingService = workspace.Services.GetRequiredService(); + _threadingContext = threadingContext; + _shutdownToken = shutdownToken; + _synchronizeWorkspaceQueue = new AsyncBatchingWorkQueue( DelayTimeSpan.NearImmediate, SynchronizePrimaryWorkspaceAsync, listener, shutdownToken); - // Text changes and active doc info are tiny messages. So attempt to send them immediately. Just batching - // things up if we get a flurry of notifications. - _textChangeQueue = new AsyncBatchingWorkQueue<(Document oldDocument, Document newDocument)>( - TimeSpan.Zero, - SynchronizeTextChangesAsync, - listener, - shutdownToken); - _synchronizeActiveDocumentQueue = new AsyncBatchingWorkQueue( TimeSpan.Zero, SynchronizeActiveDocumentAsync, @@ -161,7 +150,7 @@ private void OnWorkspaceChangedImmediate(object? sender, WorkspaceChangeEventArg var oldDocument = e.OldSolution.GetDocument(e.DocumentId); var newDocument = e.NewSolution.GetDocument(e.DocumentId); if (oldDocument != null && newDocument != null) - _textChangeQueue.AddWork((oldDocument, newDocument)); + DispatchSynchronizeTextChanges(oldDocument, newDocument); } } @@ -207,57 +196,51 @@ await client.TryInvokeAsync( cancellationToken).ConfigureAwait(false); } - private async ValueTask SynchronizeTextChangesAsync( - ImmutableSegmentedList<(Document oldDocument, Document newDocument)> values, - CancellationToken cancellationToken) + private void DispatchSynchronizeTextChanges( + Document oldDocument, + Document newDocument) { - var client = await RemoteHostClient.TryGetClientAsync(_workspace, cancellationToken).ConfigureAwait(false); - if (client == null) - return; - - // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this - // pushing text change worked or not doesn't affect feature's functionality. - // - // this basically see whether it can cheaply find out text changes between 2 snapshots, if it can, it will - // send out that text changes to remote side. - // - // the remote side, once got the text change, will again see whether it can use that text change information - // without any high cost and create new snapshot from it. - // - // otherwise, it will do the normal behavior of getting full text from VS side. this optimization saves - // times we need to do full text synchronization for typing scenario. - using var _ = ArrayBuilder<(DocumentId id, Checksum textChecksum, ImmutableArray changes, Checksum newTextChecksum)>.GetInstance(out var builder); - - foreach (var (oldDocument, newDocument) in values) + _ = _threadingContext.JoinableTaskFactory.RunAsync(async () => { + var client = await RemoteHostClient.TryGetClientAsync(_workspace, _shutdownToken).ConfigureAwait(false); + if (client == null) + return; + + // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this + // pushing text change worked or not doesn't affect feature's functionality. + // + // this basically see whether it can cheaply find out text changes between 2 snapshots, if it can, it will + // send out that text changes to remote side. + // + // the remote side, once got the text change, will again see whether it can use that text change information + // without any high cost and create new snapshot from it. + // + // otherwise, it will do the normal behavior of getting full text from VS side. this optimization saves + // times we need to do full text synchronization for typing scenario. if (!oldDocument.TryGetText(out var oldText) || !newDocument.TryGetText(out var newText)) { // we only support case where text already exist - continue; + return; } // Avoid allocating text before seeing if we can bail out. var changeRanges = newText.GetChangeRanges(oldText).AsImmutable(); if (changeRanges.Length == 0) - continue; + return; // no benefit here. pulling from remote host is more efficient if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length) - continue; + return; - var state = await oldDocument.State.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false); - var newState = await newDocument.State.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false); + var state = await oldDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); + var newState = await newDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); var textChanges = newText.GetTextChanges(oldText).AsImmutable(); - builder.Add((oldDocument.Id, state.Text, textChanges, newState.Text)); - } - if (builder.Count == 0) - return; - - await client.TryInvokeAsync( - (service, cancellationToken) => service.SynchronizeTextChangesAsync(builder.ToImmutableAndClear(), cancellationToken), - cancellationToken).ConfigureAwait(false); + await client.TryInvokeAsync( + (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, state.Text, textChanges, newState.Text, cancellationToken), + _shutdownToken).ConfigureAwait(false); + }); } } diff --git a/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs b/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs index 091e61d5a8438..9a40288a65cb1 100644 --- a/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs +++ b/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs @@ -7,6 +7,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Host; using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.Remote; @@ -22,10 +23,11 @@ namespace Microsoft.VisualStudio.LanguageServices.Remote; [method: ImportingConstructor] [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] internal sealed class VisualStudioWorkspaceServiceHubConnector( - IAsynchronousOperationListenerProvider listenerProvider) : IEventListener, IEventListenerStoppable + IAsynchronousOperationListenerProvider listenerProvider, IThreadingContext threadingContext) : IEventListener, IEventListenerStoppable { private readonly IAsynchronousOperationListenerProvider _listenerProvider = listenerProvider; private readonly CancellationTokenSource _disposalCancellationSource = new(); + private readonly IThreadingContext _threadingContext = threadingContext; private Task? _remoteClientInitializationTask; private SolutionChecksumUpdater? _checksumUpdater; @@ -38,7 +40,7 @@ public void StartListening(Workspace workspace, object serviceOpt) } // only push solution snapshot from primary (VS) workspace: - _checksumUpdater = new SolutionChecksumUpdater(workspace, _listenerProvider, _disposalCancellationSource.Token); + _checksumUpdater = new SolutionChecksumUpdater(workspace, _listenerProvider, _threadingContext, _disposalCancellationSource.Token); // start launching remote process, so that the first service that needs it doesn't need to wait for it: var service = workspace.Services.GetRequiredService(); diff --git a/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs b/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs index 9fa401c1f4d34..0c1aeae5ce70a 100644 --- a/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs +++ b/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs @@ -8,6 +8,7 @@ using System.Reflection; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.Remote.Testing; using Microsoft.CodeAnalysis.Shared.TestHooks; @@ -35,8 +36,9 @@ public async Task UpdaterService() var exportProvider = workspace.Services.SolutionServices.ExportProvider; var listenerProvider = exportProvider.GetExportedValue(); var globalOptions = exportProvider.GetExportedValue(); + var threadingContext = exportProvider.GetExportedValue(); - var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, CancellationToken.None); + var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, threadingContext, CancellationToken.None); var service = workspace.Services.GetRequiredService(); // make sure client is ready diff --git a/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs b/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs index 2626d6f12bbf8..f061e876b2ab5 100644 --- a/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs +++ b/src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs @@ -95,7 +95,7 @@ public async Task TestRemoteHostTextSynchronize() // sync await client.TryInvokeAsync( - (service, cancellationToken) => service.SynchronizeTextChangesAsync([(oldDocument.Id, oldState.Text, newText.GetTextChanges(oldText).AsImmutable(), newState.Text)], cancellationToken), + (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, oldState.Text, newText.GetTextChanges(oldText).AsImmutable(), newState.Text, cancellationToken), CancellationToken.None); // check that text already exist in remote side diff --git a/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs b/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs index 80ffa6718ef66..66d07db6e18fa 100644 --- a/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs +++ b/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs @@ -13,6 +13,7 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Editor.Test; using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.Host; @@ -1165,7 +1166,8 @@ public async Task ValidateUpdaterInformsRemoteWorkspaceOfActiveDocument(bool upd // By creating a checksum updater, we should notify the remote workspace of the active document. var listenerProvider = workspace.ExportProvider.GetExportedValue(); - var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, CancellationToken.None); + var threadingContext = workspace.ExportProvider.GetExportedValue(); + var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, threadingContext, CancellationToken.None); var assetProvider = await GetAssetProviderAsync(workspace, remoteWorkspace, solution); @@ -1219,7 +1221,8 @@ class Program2 objectReference2_step1.AssertReleased(); var listenerProvider = workspace.ExportProvider.GetExportedValue(); - var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, CancellationToken.None); + var threadingContext = workspace.ExportProvider.GetExportedValue(); + var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, threadingContext, CancellationToken.None); var assetProvider = await GetAssetProviderAsync(workspace, remoteWorkspace, solution); diff --git a/src/Workspaces/Remote/Core/IRemoteAssetSynchronizationService.cs b/src/Workspaces/Remote/Core/IRemoteAssetSynchronizationService.cs index 079bc05f93d52..1574b002ed4ad 100644 --- a/src/Workspaces/Remote/Core/IRemoteAssetSynchronizationService.cs +++ b/src/Workspaces/Remote/Core/IRemoteAssetSynchronizationService.cs @@ -26,7 +26,10 @@ internal interface IRemoteAssetSynchronizationService /// entire contents of the file over. /// ValueTask SynchronizeTextChangesAsync( - ImmutableArray<(DocumentId documentId, Checksum baseTextChecksum, ImmutableArray textChanges, Checksum newTextChecksum)> changes, + DocumentId documentId, + Checksum baseTextChecksum, + ImmutableArray textChanges, + Checksum newTextChecksum, CancellationToken cancellationToken); /// diff --git a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs index f6cc7c147aab3..4e7112abd6f6f 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs @@ -2,7 +2,6 @@ // 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.Generic; using System.Collections.Immutable; using System.Threading; using System.Threading.Tasks; @@ -49,7 +48,10 @@ public ValueTask SynchronizeActiveDocumentAsync(DocumentId? documentId, Cancella } public ValueTask SynchronizeTextChangesAsync( - ImmutableArray<(DocumentId documentId, Checksum baseTextChecksum, ImmutableArray textChanges, Checksum newTextChecksum)> changes, + DocumentId documentId, + Checksum baseTextChecksum, + ImmutableArray textChanges, + Checksum newTextChecksum, CancellationToken cancellationToken) { return RunServiceAsync(async cancellationToken => @@ -58,25 +60,22 @@ public ValueTask SynchronizeTextChangesAsync( using (RoslynLogger.LogBlock(FunctionId.RemoteHostService_SynchronizeTextAsync, cancellationToken)) { - foreach (var (documentId, baseTextChecksum, textChanges, newTextChecksum) in changes) + // Try to get the text associated with baseTextChecksum + var text = await TryGetSourceTextAsync(WorkspaceManager, workspace, documentId, baseTextChecksum, cancellationToken).ConfigureAwait(false); + if (text == null) { - // Try to get the text associated with baseTextChecksum - var text = await TryGetSourceTextAsync(WorkspaceManager, workspace, documentId, baseTextChecksum, cancellationToken).ConfigureAwait(false); - if (text == null) - { - // it won't bring in base text if it is not there already. - // text needed will be pulled in when there is request - continue; - } + // it won't bring in base text if it is not there already. + // text needed will be pulled in when there is request + return; + } - // Now attempt to manually apply the edit, producing the new forked text. Store that directly in - // the asset cache so that future calls to retrieve it can do so quickly, without synchronizing over - // the entire document. - var newText = text.WithChanges(textChanges); - var newSerializableText = new SerializableSourceText(newText, newTextChecksum); + // Now attempt to manually apply the edit, producing the new forked text. Store that directly in + // the asset cache so that future calls to retrieve it can do so quickly, without synchronizing over + // the entire document. + var newText = text.WithChanges(textChanges); + var newSerializableText = new SerializableSourceText(newText, newTextChecksum); - WorkspaceManager.SolutionAssetCache.GetOrAdd(newSerializableText.ContentChecksum, newSerializableText); - } + WorkspaceManager.SolutionAssetCache.GetOrAdd(newSerializableText.ContentChecksum, newSerializableText); } return; From ce1b2b724a6a408587f055834782491ef44dc794 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Mon, 27 Jan 2025 09:20:02 -0800 Subject: [PATCH 05/14] Switch a couple tests from FeaturesTestCompositions to EditorTestCompositions to allow composition to include an IThreadingContext --- .../Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs | 3 ++- .../Core/Test.Next/Services/SolutionServiceTests.cs | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs b/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs index 0c1aeae5ce70a..66c179af2caee 100644 --- a/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs +++ b/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs @@ -9,6 +9,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Editor.Shared.Utilities; +using Microsoft.CodeAnalysis.Editor.UnitTests; using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.Remote.Testing; using Microsoft.CodeAnalysis.Shared.TestHooks; @@ -23,7 +24,7 @@ namespace Microsoft.CodeAnalysis.Remote.UnitTests [Trait(Traits.Feature, Traits.Features.RemoteHost)] public class RemoteHostClientServiceFactoryTests { - private static readonly TestComposition s_composition = FeaturesTestCompositions.Features.WithTestHostParts(TestHost.OutOfProcess); + private static readonly TestComposition s_composition = EditorTestCompositions.EditorFeatures.WithTestHostParts(TestHost.OutOfProcess); private static AdhocWorkspace CreateWorkspace() => new(s_composition.GetHostServices()); diff --git a/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs b/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs index 66d07db6e18fa..a582e35aa382c 100644 --- a/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs +++ b/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs @@ -15,6 +15,7 @@ using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Editor.Test; +using Microsoft.CodeAnalysis.Editor.UnitTests; using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.Host; using Microsoft.CodeAnalysis.Remote; @@ -34,7 +35,7 @@ namespace Roslyn.VisualStudio.Next.UnitTests.Remote; [Trait(Traits.Feature, Traits.Features.RemoteHost)] public class SolutionServiceTests { - private static readonly TestComposition s_composition = FeaturesTestCompositions.Features.WithTestHostParts(TestHost.OutOfProcess); + private static readonly TestComposition s_composition = EditorTestCompositions.EditorFeatures.WithTestHostParts(TestHost.OutOfProcess); private static readonly TestComposition s_compositionWithFirstDocumentIsActiveAndVisible = s_composition.AddParts(typeof(FirstDocumentIsActiveAndVisibleDocumentTrackingService.Factory)); @@ -1152,7 +1153,7 @@ public async Task ValidateUpdaterInformsRemoteWorkspaceOfActiveDocument(bool upd { var code = @"class Test { void Method() { } }"; - using var workspace = TestWorkspace.CreateCSharp(code, composition: s_compositionWithFirstDocumentIsActiveAndVisible); + using var workspace = EditorTestWorkspace.CreateCSharp(code, composition: s_compositionWithFirstDocumentIsActiveAndVisible); using var remoteWorkspace = CreateRemoteWorkspace(); var solution = workspace.CurrentSolution; From 8a841d61eacf75dd1721a9f6d55ec3de02a586b7 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Mon, 27 Jan 2025 17:20:48 -0800 Subject: [PATCH 06/14] push out aggregated telemetry with success/fail counts for SolutionChecksumUpdater.DispatchSynchronizeTextChanges and RemoteAssetSynchronizationService.SynchronizeTextChangesAsync --- .../Core/Remote/SolutionChecksumUpdater.cs | 92 +++++++++++-------- .../RemoteAssetSynchronizationService.cs | 44 ++++++--- .../Compiler/Core/Log/FunctionId.cs | 2 + 3 files changed, 88 insertions(+), 50 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index f7298200f775b..182025c6039d5 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -10,6 +10,7 @@ using Microsoft.CodeAnalysis.Internal.Log; using Microsoft.CodeAnalysis.Notification; using Microsoft.CodeAnalysis.Shared.TestHooks; +using Microsoft.CodeAnalysis.Telemetry; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.Remote; @@ -202,45 +203,62 @@ private void DispatchSynchronizeTextChanges( { _ = _threadingContext.JoinableTaskFactory.RunAsync(async () => { - var client = await RemoteHostClient.TryGetClientAsync(_workspace, _shutdownToken).ConfigureAwait(false); - if (client == null) - return; - - // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this - // pushing text change worked or not doesn't affect feature's functionality. - // - // this basically see whether it can cheaply find out text changes between 2 snapshots, if it can, it will - // send out that text changes to remote side. - // - // the remote side, once got the text change, will again see whether it can use that text change information - // without any high cost and create new snapshot from it. - // - // otherwise, it will do the normal behavior of getting full text from VS side. this optimization saves - // times we need to do full text synchronization for typing scenario. - if (!oldDocument.TryGetText(out var oldText) || - !newDocument.TryGetText(out var newText)) + var ableToSync = false; + try { - // we only support case where text already exist - return; + var client = await RemoteHostClient.TryGetClientAsync(_workspace, _shutdownToken).ConfigureAwait(false); + if (client == null) + return; + + // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this + // pushing text change worked or not doesn't affect feature's functionality. + // + // this basically see whether it can cheaply find out text changes between 2 snapshots, if it can, it will + // send out that text changes to remote side. + // + // the remote side, once got the text change, will again see whether it can use that text change information + // without any high cost and create new snapshot from it. + // + // otherwise, it will do the normal behavior of getting full text from VS side. this optimization saves + // times we need to do full text synchronization for typing scenario. + if (!oldDocument.TryGetText(out var oldText) || + !newDocument.TryGetText(out var newText)) + { + // we only support case where text already exist + return; + } + + // we are able to synchronize (but might choose not to due to various optimizations) + ableToSync = true; + + // Avoid allocating text before seeing if we can bail out. + var changeRanges = newText.GetChangeRanges(oldText).AsImmutable(); + if (changeRanges.Length == 0) + return; + + // no benefit here. pulling from remote host is more efficient + if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length) + return; + + var state = await oldDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); + var newState = await newDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); + + var textChanges = newText.GetTextChanges(oldText).AsImmutable(); + + await client.TryInvokeAsync( + (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, state.Text, textChanges, newState.Text, cancellationToken), + _shutdownToken).ConfigureAwait(false); + } + finally + { + var metricName = ableToSync ? "SucceededCount" : "FailedCount"; + TelemetryLogging.LogAggregatedCounter(FunctionId.ChecksumUpdater_SynchronizeTextChangesStatus, KeyValueLogMessage.Create(m => + { + m[TelemetryLogging.KeyName] = nameof(SolutionChecksumUpdater) + "." + metricName; + m[TelemetryLogging.KeyValue] = 1L; + m[TelemetryLogging.KeyMetricName] = metricName; + })); } - - // Avoid allocating text before seeing if we can bail out. - var changeRanges = newText.GetChangeRanges(oldText).AsImmutable(); - if (changeRanges.Length == 0) - return; - - // no benefit here. pulling from remote host is more efficient - if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length) - return; - - var state = await oldDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); - var newState = await newDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); - - var textChanges = newText.GetTextChanges(oldText).AsImmutable(); - - await client.TryInvokeAsync( - (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, state.Text, textChanges, newState.Text, cancellationToken), - _shutdownToken).ConfigureAwait(false); }); } } diff --git a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs index 4e7112abd6f6f..c8ed96cac9b0f 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs @@ -7,6 +7,7 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis.Internal.Log; using Microsoft.CodeAnalysis.Serialization; +using Microsoft.CodeAnalysis.Telemetry; using Microsoft.CodeAnalysis.Text; using Roslyn.Utilities; using RoslynLogger = Microsoft.CodeAnalysis.Internal.Log.Logger; @@ -60,22 +61,39 @@ public ValueTask SynchronizeTextChangesAsync( using (RoslynLogger.LogBlock(FunctionId.RemoteHostService_SynchronizeTextAsync, cancellationToken)) { - // Try to get the text associated with baseTextChecksum - var text = await TryGetSourceTextAsync(WorkspaceManager, workspace, documentId, baseTextChecksum, cancellationToken).ConfigureAwait(false); - if (text == null) + var ableToSync = false; + + try { - // it won't bring in base text if it is not there already. - // text needed will be pulled in when there is request - return; - } + // Try to get the text associated with baseTextChecksum + var text = await TryGetSourceTextAsync(WorkspaceManager, workspace, documentId, baseTextChecksum, cancellationToken).ConfigureAwait(false); + if (text == null) + { + // it won't bring in base text if it is not there already. + // text needed will be pulled in when there is request + return; + } + + ableToSync = true; - // Now attempt to manually apply the edit, producing the new forked text. Store that directly in - // the asset cache so that future calls to retrieve it can do so quickly, without synchronizing over - // the entire document. - var newText = text.WithChanges(textChanges); - var newSerializableText = new SerializableSourceText(newText, newTextChecksum); + // Now attempt to manually apply the edit, producing the new forked text. Store that directly in + // the asset cache so that future calls to retrieve it can do so quickly, without synchronizing over + // the entire document. + var newText = text.WithChanges(textChanges); + var newSerializableText = new SerializableSourceText(newText, newTextChecksum); - WorkspaceManager.SolutionAssetCache.GetOrAdd(newSerializableText.ContentChecksum, newSerializableText); + WorkspaceManager.SolutionAssetCache.GetOrAdd(newSerializableText.ContentChecksum, newSerializableText); + } + finally + { + var metricName = ableToSync ? "SucceededCount" : "FailedCount"; + TelemetryLogging.LogAggregatedCounter(FunctionId.RemoteHostService_SynchronizeTextAsyncStatus, KeyValueLogMessage.Create(m => + { + m[TelemetryLogging.KeyName] = nameof(RemoteAssetSynchronizationService) + "." + metricName; + m[TelemetryLogging.KeyValue] = 1L; + m[TelemetryLogging.KeyMetricName] = metricName; + })); + } } return; diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs index 7dcd74b4ca00f..f2e0fc4f14204 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs @@ -642,4 +642,6 @@ internal enum FunctionId // 900-999 for items that don't fit into other categories. Workspace_EventsImmediate = 900, + ChecksumUpdater_SynchronizeTextChangesStatus = 901, + RemoteHostService_SynchronizeTextAsyncStatus = 902, } From 64a596b36339fad12b005a6daee2c617850577fe Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Mon, 27 Jan 2025 18:28:52 -0800 Subject: [PATCH 07/14] Comment the JTF.RunAsync call use some local functions for cleanup Update some comments --- .../Core/Remote/SolutionChecksumUpdater.cs | 46 +++++++++++-------- .../Portable/Workspace/Workspace_Events.cs | 3 +- .../RemoteAssetSynchronizationService.cs | 36 +++++++-------- 3 files changed, 46 insertions(+), 39 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index 182025c6039d5..31edb60e90192 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -201,14 +201,33 @@ private void DispatchSynchronizeTextChanges( Document oldDocument, Document newDocument) { + // Attempt to inform the remote asset synchronization service as quickly as possible + // about the text changes between oldDocument and newDocument. By doing this, we can + // reduce the likelihood of the remote side encountering an unknown checksum and + // requiring a synchronization of the full document. + // This method uses JTF.RunAsync to create a fire-and-forget task. JTF.RunAsync will + // attempt to execute DispatchSynchronizeTextChangesHelperAsync synchronously if possible. + // If it is unable to finish synchronously, then we'll return to the caller without + // the task having been completed, and it will complete later. _ = _threadingContext.JoinableTaskFactory.RunAsync(async () => { - var ableToSync = false; - try + var wasSynchronized = await DispatchSynchronizeTextChangesHelperAsync().ConfigureAwait(false); + + var metricName = wasSynchronized ? "SucceededCount" : "FailedCount"; + TelemetryLogging.LogAggregatedCounter(FunctionId.ChecksumUpdater_SynchronizeTextChangesStatus, KeyValueLogMessage.Create(m => + { + m[TelemetryLogging.KeyName] = nameof(SolutionChecksumUpdater) + "." + metricName; + m[TelemetryLogging.KeyValue] = 1L; + m[TelemetryLogging.KeyMetricName] = metricName; + })); + + return; + + async Task DispatchSynchronizeTextChangesHelperAsync() { var client = await RemoteHostClient.TryGetClientAsync(_workspace, _shutdownToken).ConfigureAwait(false); if (client == null) - return; + return false; // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this // pushing text change worked or not doesn't affect feature's functionality. @@ -225,20 +244,17 @@ private void DispatchSynchronizeTextChanges( !newDocument.TryGetText(out var newText)) { // we only support case where text already exist - return; + return false; } - // we are able to synchronize (but might choose not to due to various optimizations) - ableToSync = true; - // Avoid allocating text before seeing if we can bail out. var changeRanges = newText.GetChangeRanges(oldText).AsImmutable(); if (changeRanges.Length == 0) - return; + return true; // no benefit here. pulling from remote host is more efficient if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length) - return; + return true; var state = await oldDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); var newState = await newDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); @@ -248,16 +264,8 @@ private void DispatchSynchronizeTextChanges( await client.TryInvokeAsync( (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, state.Text, textChanges, newState.Text, cancellationToken), _shutdownToken).ConfigureAwait(false); - } - finally - { - var metricName = ableToSync ? "SucceededCount" : "FailedCount"; - TelemetryLogging.LogAggregatedCounter(FunctionId.ChecksumUpdater_SynchronizeTextChangesStatus, KeyValueLogMessage.Create(m => - { - m[TelemetryLogging.KeyName] = nameof(SolutionChecksumUpdater) + "." + metricName; - m[TelemetryLogging.KeyValue] = 1L; - m[TelemetryLogging.KeyMetricName] = metricName; - })); + + return true; } }); } diff --git a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs index 9ca563c476893..9168cdb378a6f 100644 --- a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs +++ b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs @@ -45,7 +45,8 @@ public event EventHandler WorkspaceChanged /// /// An event raised *immediately* whenever the current solution is changed. Handlers - /// should be written to be very fast. + /// should be written to be very fast. Called on the same thread changing the workspace, + /// which may vary depending on the workspace. /// internal event EventHandler WorkspaceChangedImmediate { diff --git a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs index c8ed96cac9b0f..13d4274ec4e05 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs @@ -57,13 +57,23 @@ public ValueTask SynchronizeTextChangesAsync( { return RunServiceAsync(async cancellationToken => { - var workspace = GetWorkspace(); + var wasSynchronized = await SynchronizeTextChangesHelperAsync().ConfigureAwait(false); - using (RoslynLogger.LogBlock(FunctionId.RemoteHostService_SynchronizeTextAsync, cancellationToken)) + var metricName = wasSynchronized ? "SucceededCount" : "FailedCount"; + TelemetryLogging.LogAggregatedCounter(FunctionId.RemoteHostService_SynchronizeTextAsyncStatus, KeyValueLogMessage.Create(m => { - var ableToSync = false; + m[TelemetryLogging.KeyName] = nameof(RemoteAssetSynchronizationService) + "." + metricName; + m[TelemetryLogging.KeyValue] = 1L; + m[TelemetryLogging.KeyMetricName] = metricName; + })); - try + return; + + async Task SynchronizeTextChangesHelperAsync() + { + var workspace = GetWorkspace(); + + using (RoslynLogger.LogBlock(FunctionId.RemoteHostService_SynchronizeTextAsync, cancellationToken)) { // Try to get the text associated with baseTextChecksum var text = await TryGetSourceTextAsync(WorkspaceManager, workspace, documentId, baseTextChecksum, cancellationToken).ConfigureAwait(false); @@ -71,11 +81,9 @@ public ValueTask SynchronizeTextChangesAsync( { // it won't bring in base text if it is not there already. // text needed will be pulled in when there is request - return; + return false; } - ableToSync = true; - // Now attempt to manually apply the edit, producing the new forked text. Store that directly in // the asset cache so that future calls to retrieve it can do so quickly, without synchronizing over // the entire document. @@ -84,19 +92,9 @@ public ValueTask SynchronizeTextChangesAsync( WorkspaceManager.SolutionAssetCache.GetOrAdd(newSerializableText.ContentChecksum, newSerializableText); } - finally - { - var metricName = ableToSync ? "SucceededCount" : "FailedCount"; - TelemetryLogging.LogAggregatedCounter(FunctionId.RemoteHostService_SynchronizeTextAsyncStatus, KeyValueLogMessage.Create(m => - { - m[TelemetryLogging.KeyName] = nameof(RemoteAssetSynchronizationService) + "." + metricName; - m[TelemetryLogging.KeyValue] = 1L; - m[TelemetryLogging.KeyMetricName] = metricName; - })); - } - } - return; + return true; + } async static Task TryGetSourceTextAsync( RemoteWorkspaceManager workspaceManager, From d715ab9c99e49ba1e25762ddcefce29de902321a Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Mon, 27 Jan 2025 18:51:57 -0800 Subject: [PATCH 08/14] move duplicated code to helper method --- .../Portable/Workspace/Workspace_Events.cs | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs index 9168cdb378a6f..60e0a73d99101 100644 --- a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs +++ b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs @@ -82,11 +82,8 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio WorkspaceChangeEventArgs args = null; if (ev.HasHandlers) { - using (Logger.LogBlock(FunctionId.Workspace_EventsImmediate, (s, p, d, k) => $"{s.Id} - {p} - {d} {kind.ToString()}", newSolution, projectId, documentId, kind, CancellationToken.None)) - { - args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); - ev.RaiseEvent(static (handler, arg) => handler(arg.self, arg.args), (self: this, args)); - } + args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); + RaiseEventForHandlers(ev, args, isImmediate: true); } ev = GetEventHandlers(WorkspaceChangeEventName); @@ -94,17 +91,27 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio { return this.ScheduleTask(() => { - using (Logger.LogBlock(FunctionId.Workspace_Events, (s, p, d, k) => $"{s.Id} - {p} - {d} {kind.ToString()}", newSolution, projectId, documentId, kind, CancellationToken.None)) - { - args ??= new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); - ev.RaiseEvent(static (handler, arg) => handler(arg.self, arg.args), (self: this, args)); - } + args ??= new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); + RaiseEventForHandlers(ev, args, isImmediate: false); }, WorkspaceChangeEventName); } else { return Task.CompletedTask; } + + static void RaiseEventForHandlers( + EventMap.EventHandlerSet> handlers, + WorkspaceChangeEventArgs args, + bool isImmediate) + { + var functionId = isImmediate ? FunctionId.Workspace_EventsImmediate : FunctionId.Workspace_Events; + + using (Logger.LogBlock(functionId, (s, p, d, k) => $"{s.Id} - {p} - {d} {args.Kind.ToString()}", args.NewSolution, args.ProjectId, args.DocumentId, args.Kind, CancellationToken.None)) + { + handlers.RaiseEvent(static (handler, args) => handler(args.NewSolution.Workspace, args), args); + } + } } /// From c643d4bd8851846c91dcce5a59c40cd1a368f723 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Tue, 28 Jan 2025 05:46:43 -0800 Subject: [PATCH 09/14] Add assert Update comments Use string constants instead of allocating --- .../Core/Remote/SolutionChecksumUpdater.cs | 11 +++++++---- .../Core/Portable/Workspace/Workspace_Events.cs | 8 +++----- .../RemoteAssetSynchronizationService.cs | 10 ++++++++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index 31edb60e90192..6d6ee6bf35b44 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -150,6 +151,8 @@ private void OnWorkspaceChangedImmediate(object? sender, WorkspaceChangeEventArg { var oldDocument = e.OldSolution.GetDocument(e.DocumentId); var newDocument = e.NewSolution.GetDocument(e.DocumentId); + + Debug.Assert(oldDocument != null && newDocument != null); if (oldDocument != null && newDocument != null) DispatchSynchronizeTextChanges(oldDocument, newDocument); } @@ -204,11 +207,11 @@ private void DispatchSynchronizeTextChanges( // Attempt to inform the remote asset synchronization service as quickly as possible // about the text changes between oldDocument and newDocument. By doing this, we can // reduce the likelihood of the remote side encountering an unknown checksum and - // requiring a synchronization of the full document. + // requiring a synchronization of the full document contents. // This method uses JTF.RunAsync to create a fire-and-forget task. JTF.RunAsync will - // attempt to execute DispatchSynchronizeTextChangesHelperAsync synchronously if possible. - // If it is unable to finish synchronously, then we'll return to the caller without - // the task having been completed, and it will complete later. + // execute DispatchSynchronizeTextChangesHelperAsync synchronously until no longer + // possible. The hopefully common occurrence is that it is able to run synchronously + // all the way until it fires off the RPC call notifying the remote side of the changes. _ = _threadingContext.JoinableTaskFactory.RunAsync(async () => { var wasSynchronized = await DispatchSynchronizeTextChangesHelperAsync().ConfigureAwait(false); diff --git a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs index 60e0a73d99101..16141b2ec70db 100644 --- a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs +++ b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs @@ -83,7 +83,7 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio if (ev.HasHandlers) { args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); - RaiseEventForHandlers(ev, args, isImmediate: true); + RaiseEventForHandlers(ev, args, FunctionId.Workspace_EventsImmediate); } ev = GetEventHandlers(WorkspaceChangeEventName); @@ -92,7 +92,7 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio return this.ScheduleTask(() => { args ??= new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); - RaiseEventForHandlers(ev, args, isImmediate: false); + RaiseEventForHandlers(ev, args, FunctionId.Workspace_Events); }, WorkspaceChangeEventName); } else @@ -103,10 +103,8 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio static void RaiseEventForHandlers( EventMap.EventHandlerSet> handlers, WorkspaceChangeEventArgs args, - bool isImmediate) + FunctionId functionId) { - var functionId = isImmediate ? FunctionId.Workspace_EventsImmediate : FunctionId.Workspace_Events; - using (Logger.LogBlock(functionId, (s, p, d, k) => $"{s.Id} - {p} - {d} {args.Kind.ToString()}", args.NewSolution, args.ProjectId, args.DocumentId, args.Kind, CancellationToken.None)) { handlers.RaiseEvent(static (handler, args) => handler(args.NewSolution.Workspace, args), args); diff --git a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs index 13d4274ec4e05..73d39d3d53a58 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/AssetSynchronization/RemoteAssetSynchronizationService.cs @@ -22,6 +22,11 @@ namespace Microsoft.CodeAnalysis.Remote; internal sealed class RemoteAssetSynchronizationService(in BrokeredServiceBase.ServiceConstructionArguments arguments) : BrokeredServiceBase(in arguments), IRemoteAssetSynchronizationService { + private const string SynchronizeTextChangesAsyncSucceededMetricName = "SucceededCount"; + private const string SynchronizeTextChangesAsyncFailedMetricName = "FailedCount"; + private const string SynchronizeTextChangesAsyncSucceededKeyName = nameof(RemoteAssetSynchronizationService) + "." + SynchronizeTextChangesAsyncSucceededMetricName; + private const string SynchronizeTextChangesAsyncFailedKeyName = nameof(RemoteAssetSynchronizationService) + "." + SynchronizeTextChangesAsyncFailedMetricName; + internal sealed class Factory : FactoryBase { protected override IRemoteAssetSynchronizationService CreateService(in ServiceConstructionArguments arguments) @@ -59,10 +64,11 @@ public ValueTask SynchronizeTextChangesAsync( { var wasSynchronized = await SynchronizeTextChangesHelperAsync().ConfigureAwait(false); - var metricName = wasSynchronized ? "SucceededCount" : "FailedCount"; + var metricName = wasSynchronized ? SynchronizeTextChangesAsyncSucceededMetricName : SynchronizeTextChangesAsyncFailedMetricName; + var keyName = wasSynchronized ? SynchronizeTextChangesAsyncSucceededKeyName : SynchronizeTextChangesAsyncFailedKeyName; TelemetryLogging.LogAggregatedCounter(FunctionId.RemoteHostService_SynchronizeTextAsyncStatus, KeyValueLogMessage.Create(m => { - m[TelemetryLogging.KeyName] = nameof(RemoteAssetSynchronizationService) + "." + metricName; + m[TelemetryLogging.KeyName] = keyName; m[TelemetryLogging.KeyValue] = 1L; m[TelemetryLogging.KeyMetricName] = metricName; })); From 595fb9f1e32aed9f7b33be6c331a6c411de26f22 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Fri, 31 Jan 2025 10:06:38 -0800 Subject: [PATCH 10/14] Go ahead and catch exceptions during EventMap eventing --- .../Compiler/Core/Utilities/EventMap.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs index 35ca9ae0fc338..181dd8ceb5c88 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs @@ -161,20 +161,21 @@ public readonly void RaiseEvent(Action invoker, TArg // will raise that as a fatal exception, but OperationCancelledExceptions might be able to propagate through and fault the task we are using in the // chain. I'm choosing to use ReportWithoutCrashAndPropagate, because if our theory here is correct, it seems the first exception isn't actually // causing crashes, and so if it turns out this is a very common situation I don't want to make a often-benign situation fatal. - try + if (this.HasHandlers) { - if (this.HasHandlers) + foreach (var registry in _registries) { - foreach (var registry in _registries) + try { registry.Invoke(invoker, arg); } + catch (Exception e) when (FatalError.ReportAndCatch(e)) + { + // Catch the exception and continue as this an event handler and propagating the exception would prevent + // other handlers from executing + } } } - catch (Exception e) when (FatalError.ReportAndPropagate(e)) - { - throw ExceptionUtilities.Unreachable(); - } } } } From 019e48b21d0b922b4e121ce7334129e11a16c699 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Mon, 3 Feb 2025 06:24:56 -0800 Subject: [PATCH 11/14] change DispatchSynchronizeTextChanges to not allocate the keyname on every invocation and to not log telemetry in the case the RemoteHostClient.TryGetClientAsync call doesn't return a client. --- .../Core/Remote/SolutionChecksumUpdater.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index 6d6ee6bf35b44..87fabacf440a3 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -48,6 +48,11 @@ internal sealed class SolutionChecksumUpdater private readonly IThreadingContext _threadingContext; private readonly CancellationToken _shutdownToken; + private const string SynchronizeTextChangesStatusSucceededMetricName = "SucceededCount"; + private const string SynchronizeTextChangesStatusFailedMetricName = "FailedCount"; + private const string SynchronizeTextChangesStatusSucceededKeyName = nameof(SolutionChecksumUpdater) + "." + SynchronizeTextChangesStatusSucceededMetricName; + private const string SynchronizeTextChangesStatusFailedKeyName = nameof(SolutionChecksumUpdater) + "." + SynchronizeTextChangesStatusFailedMetricName; + public SolutionChecksumUpdater( Workspace workspace, IAsynchronousOperationListenerProvider listenerProvider, @@ -215,22 +220,29 @@ private void DispatchSynchronizeTextChanges( _ = _threadingContext.JoinableTaskFactory.RunAsync(async () => { var wasSynchronized = await DispatchSynchronizeTextChangesHelperAsync().ConfigureAwait(false); + if (wasSynchronized == null) + return; - var metricName = wasSynchronized ? "SucceededCount" : "FailedCount"; + var metricName = wasSynchronized.Value ? SynchronizeTextChangesStatusSucceededMetricName : SynchronizeTextChangesStatusFailedMetricName; + var keyName = wasSynchronized.Value ? SynchronizeTextChangesStatusSucceededKeyName : SynchronizeTextChangesStatusFailedKeyName; TelemetryLogging.LogAggregatedCounter(FunctionId.ChecksumUpdater_SynchronizeTextChangesStatus, KeyValueLogMessage.Create(m => { - m[TelemetryLogging.KeyName] = nameof(SolutionChecksumUpdater) + "." + metricName; + m[TelemetryLogging.KeyName] = keyName; m[TelemetryLogging.KeyValue] = 1L; m[TelemetryLogging.KeyMetricName] = metricName; })); return; - async Task DispatchSynchronizeTextChangesHelperAsync() + async Task DispatchSynchronizeTextChangesHelperAsync() { var client = await RemoteHostClient.TryGetClientAsync(_workspace, _shutdownToken).ConfigureAwait(false); if (client == null) - return false; + { + // null return value indicates that we were unable to synchronize the text changes, but to not log + // telemetry against that inability as turning off OOP is not a failure. + return null; + } // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this // pushing text change worked or not doesn't affect feature's functionality. From 90003c9d532f665869ce314881b53e06e412a4c5 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Mon, 3 Feb 2025 10:12:36 -0800 Subject: [PATCH 12/14] Fix bad merge --- .../Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs b/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs index e4e9811a6c975..84bb5a7e47c7e 100644 --- a/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs +++ b/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs @@ -23,7 +23,7 @@ namespace Microsoft.VisualStudio.LanguageServices.Remote; [method: ImportingConstructor] [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] internal sealed class VisualStudioWorkspaceServiceHubConnector( - IAsynchronousOperationListenerProvider listenerProvider) : IEventListener + IAsynchronousOperationListenerProvider listenerProvider, IThreadingContext threadingContext) : IEventListener { private readonly IAsynchronousOperationListenerProvider _listenerProvider = listenerProvider; private readonly CancellationTokenSource _disposalCancellationSource = new(); From d58e64916899d1fe1f74bd4d6b7cbb154ee13315 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Tue, 4 Feb 2025 17:34:54 -0800 Subject: [PATCH 13/14] just use a fire and forget task and don't use IThreadingContext to simplify the change --- .../Core/Remote/SolutionChecksumUpdater.cs | 126 +++++++++--------- ...isualStudioWorkspaceServiceHubConnector.cs | 6 +- .../RemoteHostClientServiceFactoryTests.cs | 7 +- .../Services/SolutionServiceTests.cs | 12 +- 4 files changed, 69 insertions(+), 82 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index 87fabacf440a3..98cfa4f897696 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -7,7 +7,6 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Internal.Log; using Microsoft.CodeAnalysis.Notification; using Microsoft.CodeAnalysis.Shared.TestHooks; @@ -45,7 +44,6 @@ internal sealed class SolutionChecksumUpdater private readonly object _gate = new(); private bool _isSynchronizeWorkspacePaused; - private readonly IThreadingContext _threadingContext; private readonly CancellationToken _shutdownToken; private const string SynchronizeTextChangesStatusSucceededMetricName = "SucceededCount"; @@ -56,7 +54,6 @@ internal sealed class SolutionChecksumUpdater public SolutionChecksumUpdater( Workspace workspace, IAsynchronousOperationListenerProvider listenerProvider, - IThreadingContext threadingContext, CancellationToken shutdownToken) { var listener = listenerProvider.GetListener(FeatureAttribute.SolutionChecksumUpdater); @@ -66,7 +63,6 @@ public SolutionChecksumUpdater( _workspace = workspace; _documentTrackingService = workspace.Services.GetRequiredService(); - _threadingContext = threadingContext; _shutdownToken = shutdownToken; _synchronizeWorkspaceQueue = new AsyncBatchingWorkQueue( @@ -158,8 +154,10 @@ private void OnWorkspaceChangedImmediate(object? sender, WorkspaceChangeEventArg var newDocument = e.NewSolution.GetDocument(e.DocumentId); Debug.Assert(oldDocument != null && newDocument != null); + + // Fire-and-forget to notify remote side of this document change event as quickly as possible. if (oldDocument != null && newDocument != null) - DispatchSynchronizeTextChanges(oldDocument, newDocument); + _ = DispatchSynchronizeTextChangesAsync(oldDocument, newDocument).ReportNonFatalErrorAsync(); } } @@ -205,7 +203,7 @@ await client.TryInvokeAsync( cancellationToken).ConfigureAwait(false); } - private void DispatchSynchronizeTextChanges( + private async Task DispatchSynchronizeTextChangesAsync( Document oldDocument, Document newDocument) { @@ -217,71 +215,69 @@ private void DispatchSynchronizeTextChanges( // execute DispatchSynchronizeTextChangesHelperAsync synchronously until no longer // possible. The hopefully common occurrence is that it is able to run synchronously // all the way until it fires off the RPC call notifying the remote side of the changes. - _ = _threadingContext.JoinableTaskFactory.RunAsync(async () => + var wasSynchronized = await DispatchSynchronizeTextChangesHelperAsync().ConfigureAwait(false); + if (wasSynchronized == null) + return; + + // Update aggregated telemetry with success status of sending the synchronization data. + var metricName = wasSynchronized.Value ? SynchronizeTextChangesStatusSucceededMetricName : SynchronizeTextChangesStatusFailedMetricName; + var keyName = wasSynchronized.Value ? SynchronizeTextChangesStatusSucceededKeyName : SynchronizeTextChangesStatusFailedKeyName; + TelemetryLogging.LogAggregatedCounter(FunctionId.ChecksumUpdater_SynchronizeTextChangesStatus, KeyValueLogMessage.Create(m => { - var wasSynchronized = await DispatchSynchronizeTextChangesHelperAsync().ConfigureAwait(false); - if (wasSynchronized == null) - return; + m[TelemetryLogging.KeyName] = keyName; + m[TelemetryLogging.KeyValue] = 1L; + m[TelemetryLogging.KeyMetricName] = metricName; + })); - var metricName = wasSynchronized.Value ? SynchronizeTextChangesStatusSucceededMetricName : SynchronizeTextChangesStatusFailedMetricName; - var keyName = wasSynchronized.Value ? SynchronizeTextChangesStatusSucceededKeyName : SynchronizeTextChangesStatusFailedKeyName; - TelemetryLogging.LogAggregatedCounter(FunctionId.ChecksumUpdater_SynchronizeTextChangesStatus, KeyValueLogMessage.Create(m => - { - m[TelemetryLogging.KeyName] = keyName; - m[TelemetryLogging.KeyValue] = 1L; - m[TelemetryLogging.KeyMetricName] = metricName; - })); + return; - return; + async Task DispatchSynchronizeTextChangesHelperAsync() + { + var client = await RemoteHostClient.TryGetClientAsync(_workspace, _shutdownToken).ConfigureAwait(false); + if (client == null) + { + // null return value indicates that we were unable to synchronize the text changes, but to not log + // telemetry against that inability as turning off OOP is not a failure. + return null; + } - async Task DispatchSynchronizeTextChangesHelperAsync() + // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this + // pushing text change worked or not doesn't affect feature's functionality. + // + // this basically see whether it can cheaply find out text changes between 2 snapshots, if it can, it will + // send out that text changes to remote side. + // + // the remote side, once got the text change, will again see whether it can use that text change information + // without any high cost and create new snapshot from it. + // + // otherwise, it will do the normal behavior of getting full text from VS side. this optimization saves + // times we need to do full text synchronization for typing scenario. + if (!oldDocument.TryGetText(out var oldText) || + !newDocument.TryGetText(out var newText)) { - var client = await RemoteHostClient.TryGetClientAsync(_workspace, _shutdownToken).ConfigureAwait(false); - if (client == null) - { - // null return value indicates that we were unable to synchronize the text changes, but to not log - // telemetry against that inability as turning off OOP is not a failure. - return null; - } - - // this pushes text changes to the remote side if it can. this is purely perf optimization. whether this - // pushing text change worked or not doesn't affect feature's functionality. - // - // this basically see whether it can cheaply find out text changes between 2 snapshots, if it can, it will - // send out that text changes to remote side. - // - // the remote side, once got the text change, will again see whether it can use that text change information - // without any high cost and create new snapshot from it. - // - // otherwise, it will do the normal behavior of getting full text from VS side. this optimization saves - // times we need to do full text synchronization for typing scenario. - if (!oldDocument.TryGetText(out var oldText) || - !newDocument.TryGetText(out var newText)) - { - // we only support case where text already exist - return false; - } - - // Avoid allocating text before seeing if we can bail out. - var changeRanges = newText.GetChangeRanges(oldText).AsImmutable(); - if (changeRanges.Length == 0) - return true; - - // no benefit here. pulling from remote host is more efficient - if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length) - return true; - - var state = await oldDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); - var newState = await newDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); - - var textChanges = newText.GetTextChanges(oldText).AsImmutable(); - - await client.TryInvokeAsync( - (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, state.Text, textChanges, newState.Text, cancellationToken), - _shutdownToken).ConfigureAwait(false); + // we only support case where text already exist + return false; + } + // Avoid allocating text before seeing if we can bail out. + var changeRanges = newText.GetChangeRanges(oldText).AsImmutable(); + if (changeRanges.Length == 0) return true; - } - }); + + // no benefit here. pulling from remote host is more efficient + if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length) + return true; + + var state = await oldDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); + var newState = await newDocument.State.GetStateChecksumsAsync(_shutdownToken).ConfigureAwait(false); + + var textChanges = newText.GetTextChanges(oldText).AsImmutable(); + + await client.TryInvokeAsync( + (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, state.Text, textChanges, newState.Text, cancellationToken), + _shutdownToken).ConfigureAwait(false); + + return true; + } } } diff --git a/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs b/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs index 84bb5a7e47c7e..0ef6b7bdbe959 100644 --- a/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs +++ b/src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs @@ -7,7 +7,6 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Host; using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.Remote; @@ -23,11 +22,10 @@ namespace Microsoft.VisualStudio.LanguageServices.Remote; [method: ImportingConstructor] [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] internal sealed class VisualStudioWorkspaceServiceHubConnector( - IAsynchronousOperationListenerProvider listenerProvider, IThreadingContext threadingContext) : IEventListener + IAsynchronousOperationListenerProvider listenerProvider) : IEventListener { private readonly IAsynchronousOperationListenerProvider _listenerProvider = listenerProvider; private readonly CancellationTokenSource _disposalCancellationSource = new(); - private readonly IThreadingContext _threadingContext = threadingContext; private Task? _remoteClientInitializationTask; private SolutionChecksumUpdater? _checksumUpdater; @@ -40,7 +38,7 @@ public void StartListening(Workspace workspace) } // only push solution snapshot from primary (VS) workspace: - _checksumUpdater = new SolutionChecksumUpdater(workspace, _listenerProvider, _threadingContext, _disposalCancellationSource.Token); + _checksumUpdater = new SolutionChecksumUpdater(workspace, _listenerProvider, _disposalCancellationSource.Token); // start launching remote process, so that the first service that needs it doesn't need to wait for it: var service = workspace.Services.GetRequiredService(); diff --git a/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs b/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs index 66c179af2caee..9fa401c1f4d34 100644 --- a/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs +++ b/src/VisualStudio/Core/Test.Next/Remote/RemoteHostClientServiceFactoryTests.cs @@ -8,8 +8,6 @@ using System.Reflection; using System.Threading; using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Editor.Shared.Utilities; -using Microsoft.CodeAnalysis.Editor.UnitTests; using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.Remote.Testing; using Microsoft.CodeAnalysis.Shared.TestHooks; @@ -24,7 +22,7 @@ namespace Microsoft.CodeAnalysis.Remote.UnitTests [Trait(Traits.Feature, Traits.Features.RemoteHost)] public class RemoteHostClientServiceFactoryTests { - private static readonly TestComposition s_composition = EditorTestCompositions.EditorFeatures.WithTestHostParts(TestHost.OutOfProcess); + private static readonly TestComposition s_composition = FeaturesTestCompositions.Features.WithTestHostParts(TestHost.OutOfProcess); private static AdhocWorkspace CreateWorkspace() => new(s_composition.GetHostServices()); @@ -37,9 +35,8 @@ public async Task UpdaterService() var exportProvider = workspace.Services.SolutionServices.ExportProvider; var listenerProvider = exportProvider.GetExportedValue(); var globalOptions = exportProvider.GetExportedValue(); - var threadingContext = exportProvider.GetExportedValue(); - var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, threadingContext, CancellationToken.None); + var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, CancellationToken.None); var service = workspace.Services.GetRequiredService(); // make sure client is ready diff --git a/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs b/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs index a582e35aa382c..80ffa6718ef66 100644 --- a/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs +++ b/src/VisualStudio/Core/Test.Next/Services/SolutionServiceTests.cs @@ -13,9 +13,7 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; -using Microsoft.CodeAnalysis.Editor.Shared.Utilities; using Microsoft.CodeAnalysis.Editor.Test; -using Microsoft.CodeAnalysis.Editor.UnitTests; using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.Host; using Microsoft.CodeAnalysis.Remote; @@ -35,7 +33,7 @@ namespace Roslyn.VisualStudio.Next.UnitTests.Remote; [Trait(Traits.Feature, Traits.Features.RemoteHost)] public class SolutionServiceTests { - private static readonly TestComposition s_composition = EditorTestCompositions.EditorFeatures.WithTestHostParts(TestHost.OutOfProcess); + private static readonly TestComposition s_composition = FeaturesTestCompositions.Features.WithTestHostParts(TestHost.OutOfProcess); private static readonly TestComposition s_compositionWithFirstDocumentIsActiveAndVisible = s_composition.AddParts(typeof(FirstDocumentIsActiveAndVisibleDocumentTrackingService.Factory)); @@ -1153,7 +1151,7 @@ public async Task ValidateUpdaterInformsRemoteWorkspaceOfActiveDocument(bool upd { var code = @"class Test { void Method() { } }"; - using var workspace = EditorTestWorkspace.CreateCSharp(code, composition: s_compositionWithFirstDocumentIsActiveAndVisible); + using var workspace = TestWorkspace.CreateCSharp(code, composition: s_compositionWithFirstDocumentIsActiveAndVisible); using var remoteWorkspace = CreateRemoteWorkspace(); var solution = workspace.CurrentSolution; @@ -1167,8 +1165,7 @@ public async Task ValidateUpdaterInformsRemoteWorkspaceOfActiveDocument(bool upd // By creating a checksum updater, we should notify the remote workspace of the active document. var listenerProvider = workspace.ExportProvider.GetExportedValue(); - var threadingContext = workspace.ExportProvider.GetExportedValue(); - var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, threadingContext, CancellationToken.None); + var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, CancellationToken.None); var assetProvider = await GetAssetProviderAsync(workspace, remoteWorkspace, solution); @@ -1222,8 +1219,7 @@ class Program2 objectReference2_step1.AssertReleased(); var listenerProvider = workspace.ExportProvider.GetExportedValue(); - var threadingContext = workspace.ExportProvider.GetExportedValue(); - var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, threadingContext, CancellationToken.None); + var checksumUpdater = new SolutionChecksumUpdater(workspace, listenerProvider, CancellationToken.None); var assetProvider = await GetAssetProviderAsync(workspace, remoteWorkspace, solution); From 2cfcd65e9eaffb9876a31ba6638c2c268d211b86 Mon Sep 17 00:00:00 2001 From: Todd Grunke Date: Wed, 5 Feb 2025 19:44:28 -0800 Subject: [PATCH 14/14] Try out a Task.Yield to alleviate the concerns about doing a large amount of work on the calling (UI) thread --- .../Core/Remote/SolutionChecksumUpdater.cs | 25 +++++++++---------- .../Portable/Workspace/Workspace_Events.cs | 10 +++----- .../Compiler/Core/Utilities/EventMap.cs | 6 ----- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs index 98cfa4f897696..0a8775de3acdb 100644 --- a/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs +++ b/src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs @@ -3,12 +3,12 @@ // See the LICENSE file in the project root for more information. using System; -using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Internal.Log; using Microsoft.CodeAnalysis.Notification; +using Microsoft.CodeAnalysis.Shared.Extensions; using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CodeAnalysis.Telemetry; using Roslyn.Utilities; @@ -150,14 +150,13 @@ private void OnWorkspaceChangedImmediate(object? sender, WorkspaceChangeEventArg { if (e.Kind == WorkspaceChangeKind.DocumentChanged) { - var oldDocument = e.OldSolution.GetDocument(e.DocumentId); - var newDocument = e.NewSolution.GetDocument(e.DocumentId); + var documentId = e.DocumentId!; + var oldDocument = e.OldSolution.GetRequiredDocument(documentId); + var newDocument = e.NewSolution.GetRequiredDocument(documentId); - Debug.Assert(oldDocument != null && newDocument != null); - - // Fire-and-forget to notify remote side of this document change event as quickly as possible. - if (oldDocument != null && newDocument != null) - _ = DispatchSynchronizeTextChangesAsync(oldDocument, newDocument).ReportNonFatalErrorAsync(); + // Fire-and-forget to dispatch notification of this document change event to the remote side + // and return to the caller as quickly as possible. + _ = DispatchSynchronizeTextChangesAsync(oldDocument, newDocument).ReportNonFatalErrorAsync(); } } @@ -207,14 +206,14 @@ private async Task DispatchSynchronizeTextChangesAsync( Document oldDocument, Document newDocument) { - // Attempt to inform the remote asset synchronization service as quickly as possible + // Explicitly force a yield point here to ensure this method returns to the caller immediately and that + // all work is done off the calling thread. + await Task.Yield().ConfigureAwait(false); + + // Inform the remote asset synchronization service as quickly as possible // about the text changes between oldDocument and newDocument. By doing this, we can // reduce the likelihood of the remote side encountering an unknown checksum and // requiring a synchronization of the full document contents. - // This method uses JTF.RunAsync to create a fire-and-forget task. JTF.RunAsync will - // execute DispatchSynchronizeTextChangesHelperAsync synchronously until no longer - // possible. The hopefully common occurrence is that it is able to run synchronously - // all the way until it fires off the RPC call notifying the remote side of the changes. var wasSynchronized = await DispatchSynchronizeTextChangesHelperAsync().ConfigureAwait(false); if (wasSynchronized == null) return; diff --git a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs index 16141b2ec70db..5811a90a8cfb8 100644 --- a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs +++ b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs @@ -78,20 +78,16 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio projectId = documentId.ProjectId; } + var args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); + var ev = GetEventHandlers(WorkspaceChangedImmediateEventName); - WorkspaceChangeEventArgs args = null; - if (ev.HasHandlers) - { - args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); - RaiseEventForHandlers(ev, args, FunctionId.Workspace_EventsImmediate); - } + RaiseEventForHandlers(ev, args, FunctionId.Workspace_EventsImmediate); ev = GetEventHandlers(WorkspaceChangeEventName); if (ev.HasHandlers) { return this.ScheduleTask(() => { - args ??= new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId); RaiseEventForHandlers(ev, args, FunctionId.Workspace_Events); }, WorkspaceChangeEventName); } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs index 181dd8ceb5c88..a0dac4f2f76ed 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/EventMap.cs @@ -155,12 +155,6 @@ public readonly bool HasHandlers public readonly void RaiseEvent(Action invoker, TArg arg) { - // The try/catch here is to find additional telemetry for https://devdiv.visualstudio.com/DevDiv/_queries/query/71ee8553-7220-4b2a-98cf-20edab701fd1/. - // We've realized there's a problem with our eventing, where if an exception is encountered while calling into subscribers to Workspace events, - // we won't notify all of the callers. The expectation is such an exception would be thrown to the SafeStartNew in the workspace's event queue that - // will raise that as a fatal exception, but OperationCancelledExceptions might be able to propagate through and fault the task we are using in the - // chain. I'm choosing to use ReportWithoutCrashAndPropagate, because if our theory here is correct, it seems the first exception isn't actually - // causing crashes, and so if it turns out this is a very common situation I don't want to make a often-benign situation fatal. if (this.HasHandlers) { foreach (var registry in _registries)