-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
{ | ||
_workspace = workspace; | ||
_foregroundAffinitization = new ForegroundThreadAffinitizedObject(workspace._threadingContext, assertIsForeground: true); | ||
_asyncOperationListener = componentModel.GetService<IAsynchronousOperationListenerProvider>().GetListener(FeatureAttribute.Workspace); | ||
_runningDocumentTableEventTracker = new RunningDocumentTableEventTracker(workspace._threadingContext, | ||
componentModel.GetService<IVsEditorAdaptersFactoryService>(), runningDocumentTable, this); | ||
|
||
_closeDocumentQueue = new AsyncBatchingWorkQueue<string>( | ||
TimeSpan.FromMilliseconds(250), |
There was a problem hiding this comment.
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.
_workspace.ApplyChangeToWorkspace(w => | ||
// Don't do expensive workspace work on the UI thread. Queue this up to be processed in the BG. | ||
_closeDocumentQueue.AddWork(moniker); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review with whitespace off.
{ | ||
var documentIds = w.CurrentSolution.GetDocumentIdsWithFilePath(moniker); | ||
if (documentIds.IsDefaultOrEmpty) | ||
foreach (var moniker in monikers) |
There was a problem hiding this comment.
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.
...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some races here which I've called out. I think the naïve approach of just expanding ApplyChangesToWorkspaceAsync to cover everything might be fine and easy to do.
...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs
Outdated
Show resolved
Hide resolved
...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
private void TryOpeningDocumentsForMoniker(string moniker, ITextBuffer textBuffer, IVsHierarchy? hierarchy) | ||
{ | ||
_foregroundAffinitization.AssertIsForeground(); | ||
|
||
_workspace.ApplyChangeToWorkspace(w => | ||
_workspaceApplicationQueue.AddWork(async () => | ||
{ | ||
var documentIds = _workspace.CurrentSolution.GetDocumentIdsWithFilePath(moniker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a potential bug here now since this CurrentSolution is not being read under the lock, and the document could get removed between here and when the lock is acquired. I see a few options:
- Just put the whole thing under the ApplyChangeToWorkspaceMaybeAsync call -- that means yes we're doing a UI thread switch while holding the lock which isn't ideal, but it's at least all async.
- Try to refactor GetActiveContextProjectIdAndWatchHierarchiesAsync to not need the candidate ProjectIds. It looks reasonably doable I think; instead of it returning the ProjectId it might just return the hierarchy and maybe the value from VSHPROPID_ActiveIntellisenseProjectContext, and then we look that up once we've acquired the lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that means yes we're doing a UI thread switch while holding the lock which isn't ideal, but it's at least all async.
That definitely worries me... and makes me feel we might have some chance of deadlock. Though i suppose it's hard to get a lock inversion here as nothing can block on the BG work here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to refactor GetActiveContextProjectIdAndWatchHierarchiesAsync to not need the candidate ProjectIds.
Trying this first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. that worked. not pretty. but it worked.
{ | ||
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to put the comment here, but note that later there is:
GetProjectWithHierarchyAndName_NoLock
Which is assuming this code is running while acquiring the workspace lock. The loss of _NoLock suffix here was what somewhat tipped me off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. good catch!
...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs
Outdated
Show resolved
Hide resolved
...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs
Outdated
Show resolved
Hide resolved
…tudioWorkspaceImpl.cs
{ | ||
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); | ||
} | ||
|
||
private void TryOpeningDocumentsForMoniker(string moniker, ITextBuffer textBuffer, IVsHierarchy? hierarchy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review with whitespace off.
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
important, all the hierarchy watch/subscribe/unsubscribe is still done synchronously on the UI thread. This was needed or else things def broke :)
neither of these touch the workspace though.
@@ -243,6 +270,51 @@ void WatchHierarchy(IVsHierarchy hierarchyToWatch) | |||
return projectIds.First(); | |||
} | |||
|
|||
private void WatchAndSubscribeToHierarchies(string moniker, IVsHierarchy? hierarchy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same logic as the method above, except that it only deals with the watching of hierarchies.
...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs
Outdated
Show resolved
Hide resolved
…tudioWorkspaceImpl.OpenFileTracker.cs
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs
Outdated
Show resolved
Hide resolved
@jasonmalinowski this is ready for another pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. Splitting up the code touching the IVsHierarchy vs. figuring out which project it was was actually a bit less icky than I imagined, so good job.
@@ -215,32 +282,19 @@ void WatchHierarchy(IVsHierarchy hierarchyToWatch) | |||
} | |||
|
|||
// We may have multiple projects with the same hierarchy, but we can use __VSHPROPID8.VSHPROPID_ActiveIntellisenseProjectContext to distinguish | |||
mappedHierarchy = hierarchy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it easier just to do this right away and then let the rest of the function universally use mappedHierarchy? Or should we just pass the hierarchy ref instead of having one in parameter and one out parameter both of which are (hopefully) in sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, i dont' understand this code at all. my primary goal was preserving the semantics of hte prior system (which seems super compplex to me) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, i think i'll do this last.
@@ -215,30 +247,51 @@ void WatchHierarchy(IVsHierarchy hierarchyToWatch) | |||
} | |||
|
|||
// We may have multiple projects with the same hierarchy, but we can use __VSHPROPID8.VSHPROPID_ActiveIntellisenseProjectContext to distinguish | |||
VisualStudioProject? project = null; | |||
if (ErrorHandler.Succeeded(hierarchy.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID8.VSHPROPID_ActiveIntellisenseProjectContext, out var contextProjectNameObject))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only supported for multi-targeting projects, so it's only supported from CPS. Other project systems (csproj, the legacy web projects, third party ones) won't.
No description provided.