Add new workspace event that gives handlers the opportunity to be processed immediately

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs

@@ -3,16 +3,14 @@
 // 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.Internal.Log;
 using Microsoft.CodeAnalysis.Notification;
-using Microsoft.CodeAnalysis.PooledObjects;
+using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Shared.TestHooks;
-using Microsoft.CodeAnalysis.Text;
+using Microsoft.CodeAnalysis.Telemetry;
 using Microsoft.CodeAnalysis.Threading;
 using Roslyn.Utilities;
 
@@ -34,13 +32,6 @@ internal sealed class SolutionChecksumUpdater
 
     private readonly IDocumentTrackingService _documentTrackingService;
 
-    /// <summary>
-    /// 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.
-    /// </summary>
-    private readonly AsyncBatchingWorkQueue<(Document oldDocument, Document newDocument)> _textChangeQueue;
-
     /// <summary>
     /// Queue for kicking off the work to synchronize the primary workspace's solution.
     /// </summary>
@@ -54,6 +45,13 @@ internal sealed class SolutionChecksumUpdater
     private readonly object _gate = new();
     private bool _isSynchronizeWorkspacePaused;
 
+    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,
@@ -66,20 +64,14 @@ public SolutionChecksumUpdater(
         _workspace = workspace;
         _documentTrackingService = workspace.Services.GetRequiredService<IDocumentTrackingService>();
 
+        _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,
@@ -88,6 +80,7 @@ public SolutionChecksumUpdater(
 
         // start listening workspace change event
         _workspace.WorkspaceChanged += OnWorkspaceChanged;
+        _workspace.WorkspaceChangedImmediate += OnWorkspaceChangedImmediate;
         _documentTrackingService.ActiveDocumentChanged += OnActiveDocumentChanged;
 
         if (_globalOperationService != null)
@@ -108,6 +101,7 @@ public void Shutdown()
 
         _documentTrackingService.ActiveDocumentChanged -= OnActiveDocumentChanged;
         _workspace.WorkspaceChanged -= OnWorkspaceChanged;
+        _workspace.WorkspaceChangedImmediate -= OnWorkspaceChangedImmediate;
 
         if (_globalOperationService != null)
         {
@@ -144,14 +138,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)
@@ -161,6 +147,20 @@ private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e)
         }
     }
 
+    private void OnWorkspaceChangedImmediate(object? sender, WorkspaceChangeEventArgs e)
+    {
+        if (e.Kind == WorkspaceChangeKind.DocumentChanged)
+        {
+            var documentId = e.DocumentId!;
+            var oldDocument = e.OldSolution.GetRequiredDocument(documentId);
+            var newDocument = e.NewSolution.GetRequiredDocument(documentId);
+
+            // 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();
+        }
+    }
+
     private void OnActiveDocumentChanged(object? sender, DocumentId? e)
         => _synchronizeActiveDocumentQueue.AddWork();
 
@@ -203,57 +203,81 @@ await client.TryInvokeAsync<IRemoteAssetSynchronizationService>(
             cancellationToken).ConfigureAwait(false);
     }
 
-    private async ValueTask SynchronizeTextChangesAsync(
-        ImmutableSegmentedList<(Document oldDocument, Document newDocument)> values,
-        CancellationToken cancellationToken)
+    private async Task DispatchSynchronizeTextChangesAsync(
+        Document oldDocument,
+        Document newDocument)
     {
-        var client = await RemoteHostClient.TryGetClientAsync(_workspace, cancellationToken).ConfigureAwait(false);
-        if (client == null)
+        // 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.
+        var wasSynchronized = await DispatchSynchronizeTextChangesHelperAsync().ConfigureAwait(false);
+        if (wasSynchronized == 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<TextChange> changes, Checksum newTextChecksum)>.GetInstance(out var builder);
-
-        foreach (var (oldDocument, newDocument) in values)
+        // 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 =>
         {
+            m[TelemetryLogging.KeyName] = keyName;
+            m[TelemetryLogging.KeyValue] = 1L;
+            m[TelemetryLogging.KeyMetricName] = metricName;
+        }));
+
+        return;
+
+        async Task<bool?> 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;
+            }
+
+            // 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 false;
             }
 
             // Avoid allocating text before seeing if we can bail out.
             var changeRanges = newText.GetChangeRanges(oldText).AsImmutable();
             if (changeRanges.Length == 0)
-                continue;
+                return true;
 
             // no benefit here. pulling from remote host is more efficient
             if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length)
-                continue;
+                return true;
 
-            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<IRemoteAssetSynchronizationService>(
+                (service, cancellationToken) => service.SynchronizeTextChangesAsync(oldDocument.Id, state.Text, textChanges, newState.Text, cancellationToken),
+                _shutdownToken).ConfigureAwait(false);
 
-        await client.TryInvokeAsync<IRemoteAssetSynchronizationService>(
-            (service, cancellationToken) => service.SynchronizeTextChangesAsync(builder.ToImmutableAndClear(), cancellationToken),
-            cancellationToken).ConfigureAwait(false);
+            return true;
+        }
     }
 }

src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs

@@ -95,7 +95,7 @@ public async Task TestRemoteHostTextSynchronize()
 
             // sync
             await client.TryInvokeAsync<IRemoteAssetSynchronizationService>(
-                (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

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,24 @@ public event EventHandler<WorkspaceChangeEventArgs> WorkspaceChanged
         }
     }
 
+    /// <summary>
+    /// An event raised *immediately* whenever the current solution is changed. Handlers
+    /// should be written to be very fast. Called on the same thread changing the workspace,
+    /// which may vary depending on the workspace.
+    /// </summary>
+    internal event EventHandler<WorkspaceChangeEventArgs> 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,22 +78,34 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio
             projectId = documentId.ProjectId;
         }
 
-        var ev = GetEventHandlers<WorkspaceChangeEventArgs>(WorkspaceChangeEventName);
+        var args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId);
+
+        var ev = GetEventHandlers<WorkspaceChangeEventArgs>(WorkspaceChangedImmediateEventName);
+        RaiseEventForHandlers(ev, args, FunctionId.Workspace_EventsImmediate);
+
+        ev = GetEventHandlers<WorkspaceChangeEventArgs>(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);
-                    ev.RaiseEvent(static (handler, arg) => handler(arg.self, arg.args), (self: this, args));
-                }
+                RaiseEventForHandlers(ev, args, FunctionId.Workspace_Events);
             }, WorkspaceChangeEventName);
         }
         else
         {
             return Task.CompletedTask;
         }
+
+        static void RaiseEventForHandlers(
+            EventMap.EventHandlerSet<EventHandler<WorkspaceChangeEventArgs>> handlers,
+            WorkspaceChangeEventArgs args,
+            FunctionId functionId)
+        {
+            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);
+            }
+        }
     }
 
     /// <summary>

src/Workspaces/Remote/Core/IRemoteAssetSynchronizationService.cs

@@ -26,7 +26,10 @@ internal interface IRemoteAssetSynchronizationService
     /// <em>entire</em> contents of the file over.
     /// </summary>
     ValueTask SynchronizeTextChangesAsync(
-        ImmutableArray<(DocumentId documentId, Checksum baseTextChecksum, ImmutableArray<TextChange> textChanges, Checksum newTextChecksum)> changes,
+        DocumentId documentId,
+        Checksum baseTextChecksum,
+        ImmutableArray<TextChange> textChanges,
+        Checksum newTextChecksum,
         CancellationToken cancellationToken);
 
     /// <summary>