Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move workspace file removal to BG to avoid UI delay #59211

Merged
merged 34 commits into from
Feb 2, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
54cdba3
Move workspace file removal to BG to avoid UI delay
CyrusNajmabadi Feb 2, 2022
d119160
Add comment
CyrusNajmabadi Feb 2, 2022
db6b1ed
Queue everything
CyrusNajmabadi Feb 2, 2022
0f77bb9
Queue it all
CyrusNajmabadi Feb 2, 2022
b3e86e6
update comment
CyrusNajmabadi Feb 2, 2022
b22230d
Move more into the lock
CyrusNajmabadi Feb 2, 2022
c345874
Simplify
CyrusNajmabadi Feb 2, 2022
fc70310
Take the lock
CyrusNajmabadi Feb 2, 2022
007dc31
reduce time
CyrusNajmabadi Feb 2, 2022
a509868
Update src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualS…
CyrusNajmabadi Feb 2, 2022
8260fdd
Do hierarchy work immediately
CyrusNajmabadi Feb 2, 2022
438ca08
Merge branch 'bgFileRemoval' of https://github.com/CyrusNajmabadi/ros…
CyrusNajmabadi Feb 2, 2022
54a0cb1
Docs
CyrusNajmabadi Feb 2, 2022
4413f35
Update doc
CyrusNajmabadi Feb 2, 2022
7a20395
Restore
CyrusNajmabadi Feb 2, 2022
7684d35
Restore
CyrusNajmabadi Feb 2, 2022
03d4c3c
Restore
CyrusNajmabadi Feb 2, 2022
d82ab2c
Restore
CyrusNajmabadi Feb 2, 2022
12c33a3
Simplify
CyrusNajmabadi Feb 2, 2022
d0703f2
move local function
CyrusNajmabadi Feb 2, 2022
130eaff
Simplify
CyrusNajmabadi Feb 2, 2022
e7f2b88
Fix doc
CyrusNajmabadi Feb 2, 2022
2eb1d23
REstore
CyrusNajmabadi Feb 2, 2022
78c3f35
Simplify
CyrusNajmabadi Feb 2, 2022
22d9490
Sort
CyrusNajmabadi Feb 2, 2022
8044f50
Restore
CyrusNajmabadi Feb 2, 2022
977cb81
Restore
CyrusNajmabadi Feb 2, 2022
c238930
Update src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualS…
CyrusNajmabadi Feb 2, 2022
8dcb8d2
Simplify
CyrusNajmabadi Feb 2, 2022
801e8fe
Merge branch 'bgFileRemoval' of https://github.com/CyrusNajmabadi/ros…
CyrusNajmabadi Feb 2, 2022
e3f6aa9
Merge branch 'main' into bgFileRemoval
CyrusNajmabadi Feb 2, 2022
a824941
Take lock once
CyrusNajmabadi Feb 2, 2022
f7d54c8
Pass cancellation token
CyrusNajmabadi Feb 2, 2022
2ea895d
Move to end
CyrusNajmabadi Feb 2, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
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;
Expand Down Expand Up @@ -37,6 +38,8 @@ public sealed class OpenFileTracker : IRunningDocumentTableEventListener

private readonly RunningDocumentTableEventTracker _runningDocumentTableEventTracker;

private readonly AsyncBatchingWorkQueue<string> _closeDocumentQueue;

#region Fields read/written to from multiple threads to track files that need to be checked

/// <summary>
Expand Down Expand Up @@ -66,8 +69,7 @@ public sealed class OpenFileTracker : IRunningDocumentTableEventListener
/// 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.
/// </summary>
private readonly MultiDictionary<string, IReferenceCountedDisposable<ICacheEntry<IVsHierarchy, HierarchyEventSink>>> _watchedHierarchiesForDocumentMoniker
= new();
private readonly MultiDictionary<string, IReferenceCountedDisposable<ICacheEntry<IVsHierarchy, HierarchyEventSink>>> _watchedHierarchiesForDocumentMoniker = new();

#endregion

Expand All @@ -83,13 +85,24 @@ private readonly MultiDictionary<string, IReferenceCountedDisposable<ICacheEntry
/// This cutoff of 10 was chosen arbitrarily and with no evidence whatsoever.</remarks>
private const int CutoffForCheckingAllRunningDocumentTableDocuments = 10;

private OpenFileTracker(VisualStudioWorkspaceImpl workspace, IVsRunningDocumentTable runningDocumentTable, IComponentModel componentModel)
private OpenFileTracker(
VisualStudioWorkspaceImpl workspace,
IThreadingContext threadingContext,
IVsRunningDocumentTable runningDocumentTable,
IComponentModel componentModel,
IAsynchronousOperationListenerProvider listenerProvider)
{
_workspace = workspace;
_foregroundAffinitization = new ForegroundThreadAffinitizedObject(workspace._threadingContext, assertIsForeground: true);
_asyncOperationListener = componentModel.GetService<IAsynchronousOperationListenerProvider>().GetListener(FeatureAttribute.Workspace);
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
_runningDocumentTableEventTracker = new RunningDocumentTableEventTracker(workspace._threadingContext,
componentModel.GetService<IVsEditorAdaptersFactoryService>(), runningDocumentTable, this);

_closeDocumentQueue = new AsyncBatchingWorkQueue<string>(
TimeSpan.FromMilliseconds(250),
Copy link
Member Author

Choose a reason for hiding this comment

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

arbitrary time. but it felt like a reasonable amount of time to hear about a lot of closes, while not takign a lot of realworld wallclock time before starting.

ProcessCloseDocumentWorkspaceActionsAsync,
listenerProvider.GetListener(FeatureAttribute.Workspace),
threadingContext.DisposalToken);
}

void IRunningDocumentTableEventListener.OnOpenDocument(string moniker, ITextBuffer textBuffer, IVsHierarchy? hierarchy, IVsWindowFrame? _)
Expand All @@ -108,15 +121,19 @@ void IRunningDocumentTableEventListener.OnRenameDocument(string newMoniker, stri
{
}

public static async Task<OpenFileTracker> CreateAsync(VisualStudioWorkspaceImpl workspace, IAsyncServiceProvider asyncServiceProvider)
public static async Task<OpenFileTracker> CreateAsync(
VisualStudioWorkspaceImpl workspace,
IThreadingContext threadingContext,
IAsyncServiceProvider asyncServiceProvider,
IAsynchronousOperationListenerProvider listenerProvider)
{
var runningDocumentTable = (IVsRunningDocumentTable?)await asyncServiceProvider.GetServiceAsync(typeof(SVsRunningDocumentTable)).ConfigureAwait(true);
Assumes.Present(runningDocumentTable);

var componentModel = (IComponentModel?)await asyncServiceProvider.GetServiceAsync(typeof(SComponentModel)).ConfigureAwait(true);
Assumes.Present(componentModel);

return new OpenFileTracker(workspace, runningDocumentTable, componentModel);
return new OpenFileTracker(workspace, threadingContext, runningDocumentTable, componentModel, listenerProvider);
}

private void TryOpeningDocumentsForMoniker(string moniker, ITextBuffer textBuffer, IVsHierarchy? hierarchy)
Copy link
Member Author

Choose a reason for hiding this comment

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

review with whitespace off.

Expand Down Expand Up @@ -301,30 +318,39 @@ private void TryClosingDocumentsForMoniker(string moniker)

UnsubscribeFromWatchedHierarchies(moniker);

_workspace.ApplyChangeToWorkspace(w =>
// Don't do expensive workspace work on the UI thread. Queue this up to be processed in the BG.
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
_closeDocumentQueue.AddWork(moniker);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

review with whitespace off.


private ValueTask ProcessCloseDocumentWorkspaceActionsAsync(ImmutableArray<string> monikers, CancellationToken _)
{
return _workspace.ApplyChangeToWorkspaceMaybeAsync(useAsync: true, w =>
{
var documentIds = w.CurrentSolution.GetDocumentIdsWithFilePath(moniker);
if (documentIds.IsDefaultOrEmpty)
foreach (var moniker in monikers)
Copy link
Member Author

Choose a reason for hiding this comment

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

i could also lift this loop outside of the _workspace.apply call. I'm not sure which is preferred. Taht would break things up into lots of little pieces of work, but would acquire/release the lock a lot more. I figure throughput is desirable here, so we jsut take the lock once and then do all of our processing at once.

{
return;
}
var documentIds = w.CurrentSolution.GetDocumentIdsWithFilePath(moniker);
if (documentIds.IsDefaultOrEmpty)
{
return;
}

foreach (var documentId in documentIds)
{
if (w.IsDocumentOpen(documentId) && !_workspace._documentsNotFromFiles.Contains(documentId))
foreach (var documentId in documentIds)
{
if (w.CurrentSolution.ContainsDocument(documentId))
if (w.IsDocumentOpen(documentId) && !_workspace._documentsNotFromFiles.Contains(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));
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));
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ public VisualStudioWorkspaceImpl(ExportProvider exportProvider, IAsyncServicePro
_textBufferFactoryService.TextBufferCreated += AddTextBufferCloneServiceToBuffer;
_projectionBufferFactoryService.ProjectionBufferCreated += AddTextBufferCloneServiceToBuffer;

_ = Task.Run(() => InitializeUIAffinitizedServicesAsync(asyncServiceProvider));
var asyncOperationListenerProvider = exportProvider.GetExportedValue<IAsynchronousOperationListenerProvider>()
_ = Task.Run(() => InitializeUIAffinitizedServicesAsync(asyncServiceProvider, asyncOperationListenerProvider));

FileChangeWatcher = exportProvider.GetExportedValue<FileChangeWatcherProvider>().Watcher;
FileWatchedReferenceFactory = exportProvider.GetExportedValue<FileWatchedPortableExecutableReferenceFactory>();
Expand All @@ -157,7 +158,7 @@ public VisualStudioWorkspaceImpl(ExportProvider exportProvider, IAsyncServicePro
this,
exportProvider.GetExportedValue<IDiagnosticAnalyzerService>(),
exportProvider.GetExportedValue<IDiagnosticUpdateSourceRegistrationService>(),
exportProvider.GetExportedValue<IAsynchronousOperationListenerProvider>(),
asyncOperationListenerProvider,
_threadingContext), isThreadSafe: true);
}

Expand Down Expand Up @@ -200,7 +201,9 @@ internal void SubscribeExternalErrorDiagnosticUpdateSourceToSolutionBuildEvents(
_isExternalErrorDiagnosticUpdateSourceSubscribedToSolutionBuildEvents = true;
}

public async Task InitializeUIAffinitizedServicesAsync(IAsyncServiceProvider asyncServiceProvider)
public async Task InitializeUIAffinitizedServicesAsync(
IAsyncServiceProvider asyncServiceProvider,
IAsynchronousOperationListenerProvider listenerProvider)
{
// Create services that are bound to the UI thread
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(_threadingContext.DisposalToken);
Expand All @@ -214,7 +217,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, asyncServiceProvider).ConfigureAwait(true);
var openFileTracker = await OpenFileTracker.CreateAsync(this, _threadingContext, asyncServiceProvider, listenerProvider).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.
Expand Down