Move workspace file removal to BG to avoid UI delay by CyrusNajmabadi · Pull Request #59211 · dotnet/roslyn

CyrusNajmabadi · 2022-02-02T00:52:13Z

No description provided.

CyrusNajmabadi · 2022-02-02T00:54:43Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

                    componentModel.GetService<IVsEditorAdaptersFactoryService>(), runningDocumentTable, this);
+
+                _closeDocumentQueue = new AsyncBatchingWorkQueue<string>(
+                    TimeSpan.FromMilliseconds(250),


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.

CyrusNajmabadi · 2022-02-02T00:55:04Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

-                _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);
+            }


review with whitespace off.

CyrusNajmabadi · 2022-02-02T00:55:54Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

                {
-                    var documentIds = w.CurrentSolution.GetDocumentIdsWithFilePath(moniker);
-                    if (documentIds.IsDefaultOrEmpty)
+                    foreach (var moniker in monikers)


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

jasonmalinowski

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

jasonmalinowski · 2022-02-02T02:04:26Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

-                _workspace.ApplyChangeToWorkspace(w =>
+                _workspaceApplicationQueue.AddWork(async () =>
                {
                    var documentIds = _workspace.CurrentSolution.GetDocumentIdsWithFilePath(moniker);


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.

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...

Try to refactor GetActiveContextProjectIdAndWatchHierarchiesAsync to not need the candidate ProjectIds.

Trying this first.

Ok. that worked. not pretty. but it worked.

jasonmalinowski · 2022-02-02T02:07:57Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

-            private ProjectId GetActiveContextProjectIdAndWatchHierarchies_NoLock(string moniker, IEnumerable<ProjectId> projectIds, IVsHierarchy? hierarchy)
+            private async Task<ProjectId> GetActiveContextProjectIdAndWatchHierarchiesAsync(string moniker, IEnumerable<ProjectId> projectIds, IVsHierarchy? hierarchy)
            {
+                await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();


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.

yup. good catch!

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

…tudioWorkspaceImpl.cs

CyrusNajmabadi · 2022-02-02T16:46:24Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

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

            private void TryOpeningDocumentsForMoniker(string moniker, ITextBuffer textBuffer, IVsHierarchy? hierarchy)


review with whitespace off.

into bgFileRemoval

CyrusNajmabadi · 2022-02-02T17:54:07Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

+                // 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);


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.

CyrusNajmabadi · 2022-02-02T18:00:00Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

                return projectIds.First();
            }

+            private void WatchAndSubscribeToHierarchies(string moniker, IVsHierarchy? hierarchy)


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

…tudioWorkspaceImpl.OpenFileTracker.cs

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

into bgFileRemoval

CyrusNajmabadi · 2022-02-02T18:22:09Z

@jasonmalinowski this is ready for another pass.

jasonmalinowski

. 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.

jasonmalinowski · 2022-02-02T20:14:59Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

                }

                // We may have multiple projects with the same hierarchy, but we can use __VSHPROPID8.VSHPROPID_ActiveIntellisenseProjectContext to distinguish
+                mappedHierarchy = hierarchy;


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?

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) :)

actually, i think i'll do this last.

jasonmalinowski · 2022-02-02T20:16:23Z

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs


                // 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)))


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.

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs

Move workspace file removal to BG to avoid UI delay

54cdba3

CyrusNajmabadi requested a review from jasonmalinowski February 2, 2022 00:52

CyrusNajmabadi requested a review from a team as a code owner February 2, 2022 00:52

ghost added the Area-IDE label Feb 2, 2022

Add comment

d119160

CyrusNajmabadi commented Feb 2, 2022

View reviewed changes

CyrusNajmabadi added 2 commits February 1, 2022 17:04

Queue everything

db6b1ed

Queue it all

0f77bb9

CyrusNajmabadi commented Feb 2, 2022

View reviewed changes

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs Outdated Show resolved Hide resolved

jasonmalinowski requested changes Feb 2, 2022

View reviewed changes

CyrusNajmabadi added 5 commits February 1, 2022 20:30

update comment

b3e86e6

Move more into the lock

b22230d

Simplify

c345874

Take the lock

fc70310

reduce time

007dc31

CyrusNajmabadi commented Feb 2, 2022

View reviewed changes

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs Outdated Show resolved Hide resolved

Update src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualS…

a509868

…tudioWorkspaceImpl.cs

CyrusNajmabadi requested a review from jasonmalinowski February 2, 2022 05:23

Do hierarchy work immediately

8260fdd

CyrusNajmabadi commented Feb 2, 2022

View reviewed changes

CyrusNajmabadi added 8 commits February 2, 2022 08:46

Merge branch 'bgFileRemoval' of https://github.com/CyrusNajmabadi/roslyn

438ca08

into bgFileRemoval

Docs

54a0cb1

Update doc

4413f35

Restore

7a20395

Restore

7684d35

Restore

03d4c3c

Restore

d82ab2c

Simplify

12c33a3

CyrusNajmabadi commented Feb 2, 2022

View reviewed changes

CyrusNajmabadi added 6 commits February 2, 2022 09:54

Fix doc

e7f2b88

REstore

2eb1d23

Simplify

78c3f35

Sort

22d9490

Restore

8044f50

Restore

977cb81

CyrusNajmabadi commented Feb 2, 2022

View reviewed changes

...ualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.OpenFileTracker.cs Outdated Show resolved Hide resolved

Update src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualS…

c238930

…tudioWorkspaceImpl.OpenFileTracker.cs

CyrusNajmabadi commented Feb 2, 2022

View reviewed changes

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 2 commits February 2, 2022 10:13

Simplify

8dcb8d2

Merge branch 'bgFileRemoval' of https://github.com/CyrusNajmabadi/roslyn

801e8fe

into bgFileRemoval

runfoapp bot mentioned this pull request Feb 2, 2022

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

jasonmalinowski approved these changes Feb 2, 2022

View reviewed changes

CyrusNajmabadi added 2 commits February 2, 2022 12:24

Merge branch 'main' into bgFileRemoval

e3f6aa9

Take lock once

a824941

CyrusNajmabadi enabled auto-merge February 2, 2022 20:29

CyrusNajmabadi added 2 commits February 2, 2022 12:32

Pass cancellation token

f7d54c8

Move to end

2ea895d

ryzngard approved these changes Feb 2, 2022

View reviewed changes

CyrusNajmabadi merged commit 998b3a3 into dotnet:main Feb 2, 2022

ghost added this to the Next milestone Feb 2, 2022

CyrusNajmabadi deleted the bgFileRemoval branch February 2, 2022 22:24

RikkiGibson mentioned this pull request Feb 3, 2022

Revert "Move workspace file removal to BG to avoid UI delay" #59241

Closed

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

jasonmalinowski mentioned this pull request Feb 11, 2022

Revert "Move workspace file removal to BG to avoid UI delay" #59472

Merged

jasonmalinowski mentioned this pull request Mar 1, 2022

Change navigation helpers to operate in two steps. #59798

Merged

Conversation

CyrusNajmabadi commented Feb 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Feb 2, 2022

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants