From 7122fb543bb2ed87187ff288d7c5ecf1b8760cc8 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Thu, 3 Feb 2022 10:02:06 -0800 Subject: [PATCH] Revert "Move workspace file removal to BG to avoid UI delay" --- ...sualStudioWorkspaceImpl.OpenFileTracker.cs | 285 +++++++----------- .../VisualStudioWorkspaceImpl.cs | 6 +- 2 files changed, 115 insertions(+), 176 deletions(-) diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs index 134a02a33cac..22bc49503955 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs @@ -7,7 +7,6 @@ using System.Collections.Immutable; using System.Diagnostics; using System.Linq; -using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Editor.Shared.Utilities; @@ -18,7 +17,6 @@ using Microsoft.VisualStudio.Shell; using Microsoft.VisualStudio.Shell.Interop; using Microsoft.VisualStudio.Text; -using Microsoft.VisualStudio.Threading; using Roslyn.Utilities; using IAsyncServiceProvider = Microsoft.VisualStudio.Shell.IAsyncServiceProvider; using Task = System.Threading.Tasks.Task; @@ -35,17 +33,10 @@ public sealed class OpenFileTracker : IRunningDocumentTableEventListener private readonly ForegroundThreadAffinitizedObject _foregroundAffinitization; private readonly VisualStudioWorkspaceImpl _workspace; - private readonly IThreadingContext _threadingContext; private readonly IAsynchronousOperationListener _asyncOperationListener; private readonly RunningDocumentTableEventTracker _runningDocumentTableEventTracker; - /// - /// Queue to process the workspace side of the document notifications we get. We process this in the BG to - /// avoid taking workspace locks on the UI thread. - /// - private readonly AsyncBatchingWorkQueue> _workspaceApplicationQueue; - #region Fields read/written to from multiple threads to track files that need to be checked /// @@ -72,11 +63,11 @@ public sealed class OpenFileTracker : IRunningDocumentTableEventListener private readonly ReferenceCountedDisposableCache _hierarchyEventSinkCache = new(); /// - /// The IVsHierarchies we have subscribed to watch for any changes to this moniker. We track this per - /// moniker, so when a document is closed we know what we have to incrementally unsubscribe from rather than - /// having to unsubscribe from everything. + /// The IVsHierarchies we have subscribed to to watch for any changes to this moniker. We track this per moniker, so + /// when a document is closed we know what we have to incrementally unsubscribe from rather than having to unsubscribe from everything. /// - private readonly MultiDictionary>> _watchedHierarchiesForDocumentMoniker = new(); + private readonly MultiDictionary>> _watchedHierarchiesForDocumentMoniker + = new(); #endregion @@ -92,40 +83,13 @@ public sealed class OpenFileTracker : IRunningDocumentTableEventListener /// This cutoff of 10 was chosen arbitrarily and with no evidence whatsoever. private const int CutoffForCheckingAllRunningDocumentTableDocuments = 10; - private OpenFileTracker( - VisualStudioWorkspaceImpl workspace, - IThreadingContext threadingContext, - IVsRunningDocumentTable runningDocumentTable, - IComponentModel componentModel) + private OpenFileTracker(VisualStudioWorkspaceImpl workspace, IVsRunningDocumentTable runningDocumentTable, IComponentModel componentModel) { _workspace = workspace; - _threadingContext = threadingContext; _foregroundAffinitization = new ForegroundThreadAffinitizedObject(workspace._threadingContext, assertIsForeground: true); _asyncOperationListener = componentModel.GetService().GetListener(FeatureAttribute.Workspace); _runningDocumentTableEventTracker = new RunningDocumentTableEventTracker(workspace._threadingContext, componentModel.GetService(), runningDocumentTable, this); - - _workspaceApplicationQueue = new AsyncBatchingWorkQueue>( - TimeSpan.FromMilliseconds(50), - ProcessWorkspaceApplicationQueueAsync, - _asyncOperationListener, - threadingContext.DisposalToken); - } - - private ValueTask ProcessWorkspaceApplicationQueueAsync(ImmutableArray> actions, CancellationToken cancellationToken) - { - // Take the workspace lock once, then apply all the changes we have while holding it. - return _workspace.ApplyChangeToWorkspaceMaybeAsync(useAsync: true, w => - { - foreach (var action in actions) - { - // Canceling here means we won't process the remaining workspace work. That's OK as we are only - // canceled during shutdown, and at that point we really don't want to be doing any unnecessary work - // anyways. - cancellationToken.ThrowIfCancellationRequested(); - action(w); - } - }, cancellationToken); } void IRunningDocumentTableEventListener.OnOpenDocument(string moniker, ITextBuffer textBuffer, IVsHierarchy? hierarchy, IVsWindowFrame? _) @@ -144,10 +108,7 @@ void IRunningDocumentTableEventListener.OnRenameDocument(string newMoniker, stri { } - public static async Task CreateAsync( - VisualStudioWorkspaceImpl workspace, - IThreadingContext threadingContext, - IAsyncServiceProvider asyncServiceProvider) + public static async Task CreateAsync(VisualStudioWorkspaceImpl workspace, IAsyncServiceProvider asyncServiceProvider) { var runningDocumentTable = (IVsRunningDocumentTable?)await asyncServiceProvider.GetServiceAsync(typeof(SVsRunningDocumentTable)).ConfigureAwait(true); Assumes.Present(runningDocumentTable); @@ -155,114 +116,85 @@ public static async Task CreateAsync( var componentModel = (IComponentModel?)await asyncServiceProvider.GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true); Assumes.Present(componentModel); - return new OpenFileTracker(workspace, threadingContext, runningDocumentTable, componentModel); + return new OpenFileTracker(workspace, runningDocumentTable, componentModel); } private void TryOpeningDocumentsForMoniker(string moniker, ITextBuffer textBuffer, IVsHierarchy? hierarchy) { _foregroundAffinitization.AssertIsForeground(); - // First clear off any existing IVsHierarchies we are watching. Any ones that still matter we will - // resubscribe to. We could be fancy and diff, but the cost is probably negligible. Then watch and - // subscribe to this hierarchy and any related ones. - UnsubscribeFromWatchedHierarchies(moniker); - WatchAndSubscribeToHierarchies(moniker, hierarchy, out var contextHierarchy, out var contextHierarchyProjectName); + _workspace.ApplyChangeToWorkspace(w => + { + var documentIds = _workspace.CurrentSolution.GetDocumentIdsWithFilePath(moniker); + if (documentIds.IsDefaultOrEmpty) + { + return; + } - _workspaceApplicationQueue.AddWork(w => + if (documentIds.All(w.IsDocumentOpen)) { - var documentIds = _workspace.CurrentSolution.GetDocumentIdsWithFilePath(moniker); - if (documentIds.IsDefaultOrEmpty) - { - return; - } + return; + } - if (documentIds.All(w.IsDocumentOpen)) - { - return; - } + ProjectId activeContextProjectId; - var activeContextProjectId = documentIds.Length == 1 - ? documentIds.Single().ProjectId - : GetActiveContextProjectId_NoLock(contextHierarchy, contextHierarchyProjectName, documentIds.SelectAsArray(d => d.ProjectId)); + if (documentIds.Length == 1) + { + activeContextProjectId = documentIds.Single().ProjectId; + } + else + { + activeContextProjectId = GetActiveContextProjectIdAndWatchHierarchies_NoLock(moniker, documentIds.Select(d => d.ProjectId), hierarchy); + } - var textContainer = textBuffer.AsTextContainer(); + var textContainer = textBuffer.AsTextContainer(); - foreach (var documentId in documentIds) + foreach (var documentId in documentIds) + { + if (!w.IsDocumentOpen(documentId) && !_workspace._documentsNotFromFiles.Contains(documentId)) { - if (!w.IsDocumentOpen(documentId) && !_workspace._documentsNotFromFiles.Contains(documentId)) + var isCurrentContext = documentId.ProjectId == activeContextProjectId; + if (w.CurrentSolution.ContainsDocument(documentId)) { - var isCurrentContext = documentId.ProjectId == activeContextProjectId; - if (w.CurrentSolution.ContainsDocument(documentId)) - { - w.OnDocumentOpened(documentId, textContainer, isCurrentContext); - } - else if (w.CurrentSolution.ContainsAdditionalDocument(documentId)) - { - w.OnAdditionalDocumentOpened(documentId, textContainer, isCurrentContext); - } - else - { - Debug.Assert(w.CurrentSolution.ContainsAnalyzerConfigDocument(documentId)); - w.OnAnalyzerConfigDocumentOpened(documentId, textContainer, isCurrentContext); - } + w.OnDocumentOpened(documentId, textContainer, isCurrentContext); + } + else if (w.CurrentSolution.ContainsAdditionalDocument(documentId)) + { + w.OnAdditionalDocumentOpened(documentId, textContainer, isCurrentContext); + } + else + { + Debug.Assert(w.CurrentSolution.ContainsAnalyzerConfigDocument(documentId)); + w.OnAnalyzerConfigDocumentOpened(documentId, textContainer, isCurrentContext); } } - }); + } + }); } - /// - /// Gets the active project ID for the given hierarchy. Only callable while holding the workspace lock. - /// - private ProjectId GetActiveContextProjectId_NoLock( - IVsHierarchy? contextHierarchy, string? contextProjectName, ImmutableArray projectIds) + private ProjectId GetActiveContextProjectIdAndWatchHierarchies_NoLock(string moniker, IEnumerable projectIds, IVsHierarchy? hierarchy) { - if (contextHierarchy == null) + _foregroundAffinitization.AssertIsForeground(); + + // First clear off any existing IVsHierarchies we are watching. Any ones that still matter we will resubscribe to. + // We could be fancy and diff, but the cost is probably negligible. + UnsubscribeFromWatchedHierarchies(moniker); + + if (hierarchy == null) { // Any item in the RDT should have a hierarchy associated; in this case we don't so there's absolutely nothing // we can do at this point. return projectIds.First(); } - // Take a snapshot of the immutable data structure here to avoid mutation underneath us - var projectToHierarchyMap = _workspace._projectToHierarchyMap; - - if (contextProjectName != null) - { - var project = _workspace.GetProjectWithHierarchyAndName_NoLock(contextHierarchy, contextProjectName); - - if (project != null && projectIds.Contains(project.Id)) - { - return project.Id; - } - } - - // At this point, we should hopefully have only one project that matches by hierarchy. If there's - // multiple, at this point we can't figure anything out better. - var matchingProjectId = projectIds.FirstOrDefault(id => projectToHierarchyMap.GetValueOrDefault(id, null) == contextHierarchy); - - if (matchingProjectId != null) + void WatchHierarchy(IVsHierarchy hierarchyToWatch) { - return matchingProjectId; + _watchedHierarchiesForDocumentMoniker.Add(moniker, _hierarchyEventSinkCache.GetOrCreate(hierarchyToWatch, static (h, self) => new HierarchyEventSink(h, self), this)); } - // If we had some trouble finding the project, we'll just pick one arbitrarily - return projectIds.First(); - } - - private void WatchAndSubscribeToHierarchies(string moniker, IVsHierarchy? hierarchy, out IVsHierarchy? mappedHierarchy, out string? mappedHierarchyName) - { - // Keep this method in sync with GetActiveContextProjectId_NoLockAsync - - _foregroundAffinitization.AssertIsForeground(); - - if (hierarchy == null) - { - // Any item in the RDT should have a hierarchy associated; in this case we don't so there's absolutely nothing - // we can do at this point. - mappedHierarchy = null; - mappedHierarchyName = null; - return; - } + // Take a snapshot of the immutable data structure here to avoid mutation underneath us + var projectToHierarchyMap = _workspace._projectToHierarchyMap; + var solution = _workspace.CurrentSolution; // We now must chase to the actual hierarchy that we know about. First, we'll chase through multiple shared asset projects if // we need to do so. @@ -284,16 +216,31 @@ private void WatchAndSubscribeToHierarchies(string moniker, IVsHierarchy? hierar // We may have multiple projects with the same hierarchy, but we can use __VSHPROPID8.VSHPROPID_ActiveIntellisenseProjectContext to distinguish if (ErrorHandler.Succeeded(hierarchy.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID8.VSHPROPID_ActiveIntellisenseProjectContext, out var contextProjectNameObject))) + { WatchHierarchy(hierarchy); - mappedHierarchy = hierarchy; - mappedHierarchyName = contextProjectNameObject as string; - return; + if (contextProjectNameObject is string contextProjectName) + { + var project = _workspace.GetProjectWithHierarchyAndName_NoLock(hierarchy, contextProjectName); - void WatchHierarchy(IVsHierarchy hierarchyToWatch) + if (project != null && projectIds.Contains(project.Id)) + { + return project.Id; + } + } + } + + // At this point, we should hopefully have only one project that maches by hierarchy. If there's multiple, at this point we can't figure anything + // out better. + var matchingProjectId = projectIds.FirstOrDefault(id => projectToHierarchyMap.GetValueOrDefault(id, null) == hierarchy); + + if (matchingProjectId != null) { - _watchedHierarchiesForDocumentMoniker.Add(moniker, _hierarchyEventSinkCache.GetOrCreate(hierarchyToWatch, static (h, self) => new HierarchyEventSink(h, self), this)); + return matchingProjectId; } + + // If we had some trouble finding the project, we'll just pick one arbitrarily + return projectIds.First(); } private void UnsubscribeFromWatchedHierarchies(string moniker) @@ -312,30 +259,22 @@ private void RefreshContextForMoniker(string moniker, IVsHierarchy hierarchy) { _foregroundAffinitization.AssertIsForeground(); - // First clear off any existing IVsHierarchies we are watching. Any ones that still matter we will - // resubscribe to. We could be fancy and diff, but the cost is probably negligible. Then watch and - // subscribe to this hierarchy and any related ones. - UnsubscribeFromWatchedHierarchies(moniker); - WatchAndSubscribeToHierarchies(moniker, hierarchy, out var contextHierarchy, out var contextProjectName); - - _workspaceApplicationQueue.AddWork(w => + _workspace.ApplyChangeToWorkspace(w => + { + var documentIds = _workspace.CurrentSolution.GetDocumentIdsWithFilePath(moniker); + if (documentIds.IsDefaultOrEmpty || documentIds.Length == 1) { - var documentIds = _workspace.CurrentSolution.GetDocumentIdsWithFilePath(moniker); - if (documentIds.IsDefaultOrEmpty || documentIds.Length == 1) - { - return; - } - - if (!documentIds.All(w.IsDocumentOpen)) - { - return; - } + return; + } - var activeProjectId = GetActiveContextProjectId_NoLock( - contextHierarchy, contextProjectName, documentIds.SelectAsArray(d => d.ProjectId)); + if (!documentIds.All(w.IsDocumentOpen)) + { + return; + } - w.OnDocumentContextUpdated(documentIds.First(d => d.ProjectId == activeProjectId)); - }); + var activeProjectId = GetActiveContextProjectIdAndWatchHierarchies_NoLock(moniker, documentIds.Select(d => d.ProjectId), hierarchy); + w.OnDocumentContextUpdated(documentIds.First(d => d.ProjectId == activeProjectId)); + }); } private void RefreshContextsForHierarchyPropertyChange(IVsHierarchy hierarchy) @@ -362,34 +301,34 @@ private void TryClosingDocumentsForMoniker(string moniker) UnsubscribeFromWatchedHierarchies(moniker); - _workspaceApplicationQueue.AddWork(w => + _workspace.ApplyChangeToWorkspace(w => + { + var documentIds = w.CurrentSolution.GetDocumentIdsWithFilePath(moniker); + if (documentIds.IsDefaultOrEmpty) { - var documentIds = w.CurrentSolution.GetDocumentIdsWithFilePath(moniker); - if (documentIds.IsDefaultOrEmpty) - { - return; - } + return; + } - foreach (var documentId in documentIds) + foreach (var documentId in documentIds) + { + if (w.IsDocumentOpen(documentId) && !_workspace._documentsNotFromFiles.Contains(documentId)) { - if (w.IsDocumentOpen(documentId) && !_workspace._documentsNotFromFiles.Contains(documentId)) + if (w.CurrentSolution.ContainsDocument(documentId)) { - if (w.CurrentSolution.ContainsDocument(documentId)) - { - w.OnDocumentClosed(documentId, new FileTextLoader(moniker, defaultEncoding: null)); - } - else if (w.CurrentSolution.ContainsAdditionalDocument(documentId)) - { - w.OnAdditionalDocumentClosed(documentId, new FileTextLoader(moniker, defaultEncoding: null)); - } - else - { - Debug.Assert(w.CurrentSolution.ContainsAnalyzerConfigDocument(documentId)); - w.OnAnalyzerConfigDocumentClosed(documentId, new FileTextLoader(moniker, defaultEncoding: null)); - } + w.OnDocumentClosed(documentId, new FileTextLoader(moniker, defaultEncoding: null)); + } + else if (w.CurrentSolution.ContainsAdditionalDocument(documentId)) + { + w.OnAdditionalDocumentClosed(documentId, new FileTextLoader(moniker, defaultEncoding: null)); + } + else + { + Debug.Assert(w.CurrentSolution.ContainsAnalyzerConfigDocument(documentId)); + w.OnAnalyzerConfigDocumentClosed(documentId, new FileTextLoader(moniker, defaultEncoding: null)); } } - }); + } + }); } /// diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs index 776ea69ba633..cf09ea1c9787 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs @@ -214,7 +214,7 @@ public async Task InitializeUIAffinitizedServicesAsync(IAsyncServiceProvider asy var solutionClosingContext = UIContext.FromUIContextGuid(VSConstants.UICONTEXT.SolutionClosing_guid); solutionClosingContext.UIContextChanged += (_, e) => _solutionClosing = e.Activated; - var openFileTracker = await OpenFileTracker.CreateAsync(this, _threadingContext, asyncServiceProvider).ConfigureAwait(true); + var openFileTracker = await OpenFileTracker.CreateAsync(this, asyncServiceProvider).ConfigureAwait(true); var memoryListener = await VirtualMemoryNotificationListener.CreateAsync(this, _threadingContext, asyncServiceProvider, _globalOptions, _threadingContext.DisposalToken).ConfigureAwait(true); // Update our fields first, so any asynchronous work that needs to use these is able to see the service. @@ -1565,9 +1565,9 @@ public void ApplyChangeToWorkspace(Action action) /// /// Applies a single operation to the workspace. should be a call to one of the protected Workspace.On* methods. /// - public async ValueTask ApplyChangeToWorkspaceMaybeAsync(bool useAsync, Action action, CancellationToken cancellationToken = default) + public async ValueTask ApplyChangeToWorkspaceMaybeAsync(bool useAsync, Action action) { - using (useAsync ? await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false) : _gate.DisposableWait(cancellationToken)) + using (useAsync ? await _gate.DisposableWaitAsync().ConfigureAwait(false) : _gate.DisposableWait()) { action(this); }