Fix more lock contention #59638

jasonmalinowski · 2022-02-18T02:14:20Z

jasonmalinowski · 2022-02-18T21:09:38Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Readme.md

@@ -0,0 +1,41 @@
+# Locking and Synchronization


This document was long overdue, so I used this as an opportunity to write this down. The content here though is not "new" nor is it changed by this PR in any way.

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Readme.md

CyrusNajmabadi · 2022-02-18T21:16:55Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Readme.md

+else holds onto a VisualStudioProjectOptionsTracker that has a lock, so we avoid any deadlocks there.
+
+VisualStudioWorkspaceImpl has a nested type OpenFileTracker that has it's own lock to guard it's own fields. It should call nothing
+outside of itself while holding that lock.


i both stronglya ppreciate this doc, while recognizing that the complexity here makes me angry.

in general, do we also have some of these comments on those types? e.g. does OpenFileTracker have comments saying "absolutely never do X"?

question still applies @jasonmalinowski

I'll add comments appropriately.

CyrusNajmabadi · 2022-02-18T21:17:55Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Readme.md

+would acquire a project lock. To this end, a few bits of information that may seem to be "project specific" are actually stored
+in maps in the VisualStudioWorkspaceImpl; specifically we maintain a list of the output paths of a project which we use to convert
+metadata references to project references. This list is maintained in the workspace itself to avoid having to reach back to a
+project and ask it for information which might violate this lock hierarchy.


this si very good to know. i wonder if it's something we could trap somehow to detect when we're doing this wrong.

CyrusNajmabadi · 2022-02-18T21:19:59Z

...udio/Core/Def/Implementation/ProjectSystem/VisualStudioProject.BatchingDocumentCollection.cs

                        foreach (var (documentId, textLoader) in documentsToChange)
                        {
-                            if (!s.Workspace.IsDocumentOpen(documentId))
+                            if (!_project._workspace.IsDocumentOpen(documentId))


here's my terror. we're in the workspace lock, calling back into the workspace. How do we know IsDocumentOpen takes no lock itself?

So if it takes the main workspace lock a second time, we'll know because it'll deadlock. 😄 I agree though yeah I don't have a good answer for how to enforce the hierarchy, generally though.

CyrusNajmabadi · 2022-02-18T21:20:50Z

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

+                if (object.Equals(field, newValue))
+                {
+                    return;
+                }


this was very confusing to read since i didn't see why this needed to be in a lock. having there be a ref-read here was not expected :)

Yeah, actual use of ref is...surprising. I can't think when I last used it before this. 😄

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

sharwell

Comments related to the threading switch do not align with the new implementation

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs

sharwell · 2022-02-18T22:31:14Z

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

@@ -160,6 +161,8 @@ public VisualStudioWorkspaceImpl(ExportProvider exportProvider, IAsyncServicePro
                    exportProvider.GetExportedValue<IDiagnosticUpdateSourceRegistrationService>(),
                    exportProvider.GetExportedValue<IAsynchronousOperationListenerProvider>(),
                    _threadingContext), isThreadSafe: true);
+
+            _workspaceListener = Services.GetRequiredService<IWorkspaceAsynchronousOperationListenerProvider>().GetListener();


💭 This could be exposed by a private protected property in Workspace:

private protected IAsynchronousOperationListener Listener => _taskQueue.Listener;

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

Blocking issue was resolved

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/ValueTaskExtensions.cs

CyrusNajmabadi · 2022-02-25T22:52:23Z

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

+                        var solutionChanges = new SolutionChangeAccumulator(s);
+                        updateSolution(solutionChanges, oldValue);
+                        return solutionChanges;
+                    }).AsTask().GetAwaiter().GetResult();


is it desirable to be sync here? as opposed to async, whcih you push up all the way to callers.

Same comment as here: https://github.com/dotnet/roslyn/pull/59638/files/1268f8f6cc2a854311b427ac6a9701fd9742286f..bcc398b3ede4373f53e2584e1ac8fe421b6ec5ac#r810348018 Sorry should have left that unresolved for you to see the answer.

Basically, the sync path here is taken by the legacy project system, that that point we're better off synchronously acquiring the locks since that'll get priority. So other than switching to be a JTF.Run() higher level, we can't really make this properly sync. And I suspect that might actually be worse for perf given SemaphoreSlim's prioritization for synchronous waters.

Somehow you broke GitHub, the current code here anyways is:

_workspace.ApplyBatchChangeToWorkspace(solutionChanges => { updateSolution(solutionChanges, oldValue); });

So the wart for the .GetResult() is gone.

CyrusNajmabadi · 2022-02-25T22:54:57Z

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

@@ -1983,38 +1983,49 @@ public void RemoveProjectOutputPath_NoLock(SolutionChangeAccumulator solutionCha

        private void RefreshMetadataReferencesForFile(object sender, string fullFilePath)
        {
-            using (_gate.DisposableWait())
+            var asyncToken = _workspaceListener.BeginAsyncOperation(nameof(RefreshMetadataReferencesForFile));
+            _threadingContext.RunWithShutdownBlockAsync(async cancellationToken =>


we have a .CompletesAsyncToken extension so you don't need the try/finally

oh wait.. is this one of those weird tasks?

The try/finally was gotten rid of per @sharwell's suggestion to use just the using var token; in this case it's returning JoinableTasks so I'm not sure our existing helpers work, and as @sharwell pointed out we don't need it since RunWithShutdownBlockAsync still runs the lambda immediately.

(GitHub should be showing this comment as outdated since this was already fixed, not sure but @CyrusNajmabadi broke it somehow.)

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs

CyrusNajmabadi · 2022-02-25T22:58:46Z

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

-            {
-                _projectToRuleSetFilePath.Add(project.Id, ruleSetFilePathFunc);
-            }
+            Contract.ThrowIfFalse(ImmutableInterlocked.TryAdd(ref _projectToRuleSetFilePath, project.Id, ruleSetFilePathFunc));


good use of Contract to preserve semantics.

CyrusNajmabadi · 2022-02-25T22:59:25Z

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

@@ -338,16 +340,14 @@ internal VisualStudioProjectTracker ProjectTracker
        // Solution Explorer in the SolutionExplorerShim, where if we just could more directly get to the rule set file it'd simplify this.
        internal override string? TryGetRuleSetPathForProject(ProjectId projectId)
        {
-            using (_gate.DisposableWait())
+            // _projectToRuleSetFilePath is immutable, so can be used outside of locks


this always worries me as i always think there's .net subtleties about all writes being observed. though i think you'r correct about safety.

CyrusNajmabadi · 2022-02-25T23:00:47Z

...ols/ExternalAccess/FSharp/Internal/VisualStudio/FSharpProjectExternalErrorReporterFactory.cs

@@ -18,9 +18,7 @@ internal static class FSharpProjectExternalErrorReporterFactory
    {
        public static IVsLanguageServiceBuildErrorReporter2 Create(ProjectId projectId, string errorCodePrefix, IServiceProvider serviceProvider)
        {
-            ThreadHelper.ThrowIfNotOnUIThread();


just to check, we're intentionally making this free threaded now?

Correct: the only reason this had a UI thread affinity was to call SubscribeExternalErrorDiagnosticUpdateSourceToSolutionBuildEvents, but that's been moved earlier when the project itself is being created.

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs

genlu · 2022-03-03T22:06:12Z

If this is for17.2 p2, then I think you need to target release/dev17.2

jasonmalinowski · 2022-03-03T22:38:02Z

@genlu The plan was since we were stabilizing integration tests, to get a green run here and then retarget once we knew this didn't have regressions. That plan...did not work. 😄

These were added for completeness but then not actually used anywhere; rather than try to maintain them just delete them.

We convert metadata references to project references if the output paths match the referenced paths. Thus if we have a solution with a bunch of metadata references, and then we're told a project has a new output path, we have to go looking to see if we can convert those metadata references to project references as well. To do that conversion, we have to acquire the global workspace lock, since any other projects that are also adding or removing metadata references at the same time need to be all coordinated. We also support the concept of "batching" when updating a project, which means a project system can make a bunch of changes at once but we'll raise only a single WorkspaceChanged event once those changes are all done; we also try to ensure that we aren't acquiring the global workspace lock until the batch is actually being applied; that ensures we can do as much in parallel at once. We also recently added the ability for the batch to be applied with an async API, so if there's another project applying changes at once we aren't going to starve the thread pool. There was one bug, however: when you started a batch, and then changed the output path of the project, that updating of the output path should have waited until the batch was applied. We'd update the property in the workspace correctly, but we immediately acquired the global workspace lock trying to look for other projects to update with project references. Functionally this was mostly fine (ignoring there's a small window in time where we converted to project references even if a project didn't have the updated output path), but it created a huge performance issue: we would be acquiring the global workspace lock synchronously, even if the caller was trying to operate in a batch that they were going to apply asynchronously. If the user got particularly unlucky, they might have a bunch of projects all being loaded or reloaded at once, and they might starve off their thread pool. The fix, conceptually, is simple: defer the conversion to project references until the global lock is actually held once we are applying the batch change. This resulted in a bit of churn; when we apply a batch we have a little helper type called a SolutionChangeAccumulator that we pass around that tracks the new Solution object, but also tracks what the resulting "kind" should be for the solution change. Thus this commit required this set of changes: 1. Convert the methods that did the conversion to/from project references to operate on a SolutionChangeAccumulator instead of directly reading CurrentSolution and directly setting the output. 2. Update the ChangeProjectOutputPath and ChangeProjectProperty functions to now operate with SolutionChangeAccumulators. 3. Update the project removal path to also now create a SolutionChangeAccumulator. 4. General refactorings (extracting/merging) of some of our core helpers where we apply changes in the end. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1473249

There's still a switch happening in CreateAndAddToWorkspaceAsync, which is where it should be and can be better limited.

One helper requires you to return a SolutionChangeAccumulator. No reason we can't just create one and pass in in rather than each caller having to create it.

…lutionBuildEvents By calling this once when we already are on the UI thread when a project is created, we're able to avoid calling it in a bunch of places which was requiring UI threads in less-intuitive places. It's also not clear to me why this is even marked as needing the UI thread at all, but we'll address that independently.

Now there's a consistent set of sync/async/maybe async.

jasonmalinowski self-assigned this Feb 18, 2022

dotnet-issue-labeler bot added the Area-IDE label Feb 18, 2022

jasonmalinowski marked this pull request as ready for review February 18, 2022 21:08

jasonmalinowski requested a review from a team as a code owner February 18, 2022 21:08

jasonmalinowski commented Feb 18, 2022

View reviewed changes

CyrusNajmabadi reviewed Feb 18, 2022

View reviewed changes

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Readme.md Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 18, 2022

View reviewed changes

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Readme.md Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 18, 2022

View reviewed changes

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

CyrusNajmabadi reviewed Feb 18, 2022

View reviewed changes

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

sharwell previously requested changes Feb 18, 2022

View reviewed changes

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs Show resolved Hide resolved

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs Show resolved Hide resolved

sharwell reviewed Feb 18, 2022

View reviewed changes

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

sharwell reviewed Feb 18, 2022

View reviewed changes

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

jasonmalinowski requested a review from a team as a code owner February 23, 2022 00:58

sharwell reviewed Feb 23, 2022

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/ValueTaskExtensions.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 23, 2022

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/ValueTaskExtensions.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 23, 2022

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/ValueTaskExtensions.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 23, 2022

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/ValueTaskExtensions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 25, 2022

View reviewed changes

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 25, 2022

View reviewed changes

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs Show resolved Hide resolved

dotnet deleted a comment from azure-pipelines bot Mar 1, 2022

dotnet deleted a comment from azure-pipelines bot Mar 2, 2022

jinujoseph added this to the 17.2.P2 milestone Mar 2, 2022

dotnet deleted a comment from azure-pipelines bot Mar 3, 2022

dotnet deleted a comment from azure-pipelines bot Mar 5, 2022

jasonmalinowski added 14 commits March 7, 2022 16:07

Delete VisualStudioProject.Add/RemoveOutputPath

d08a578

These were added for completeness but then not actually used anywhere; rather than try to maintain them just delete them.

Extract out the workspace listener to a field

7d8a600

When updating metadata references, acquire the lock asynchronously

1288b87

When creating a CPS project, don't block while setting the output path

e852615

Acquire locks asynchronously when creating projects

c428c88

Don't switch to the main thread when it's not required

6f3ce74

There's still a switch happening in CreateAndAddToWorkspaceAsync, which is where it should be and can be better limited.

Avoid taking a global lock to update _projectToRuleSetFilePath

2d5c323

Clean up some of the Apply helpers

79af2e4

One helper requires you to return a SolutionChangeAccumulator. No reason we can't just create one and pass in in rather than each caller having to create it.

Write up documentation for how the locking model works here

c798c59

Clean up our async Apply* functions

b2c1fe6

Now there's a consistent set of sync/async/maybe async.

Switch to simpler pattern with RunWithShutdownBlockAsync

848e699

Add some comments encouraging people to read the readme

36a7b1e

jasonmalinowski force-pushed the fix-more-lock-contention branch from 0ea296d to 36a7b1e Compare March 8, 2022 00:12

jasonmalinowski merged commit fa8f6bc into dotnet:main Mar 8, 2022

ghost modified the milestones: 17.2.P2, Next Mar 8, 2022

jasonmalinowski deleted the fix-more-lock-contention branch March 8, 2022 20:08

jasonmalinowski mentioned this pull request Mar 12, 2022

Backport lock contention fixes to 17.1 #60130

Closed

allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix more lock contention #59638

Fix more lock contention #59638

jasonmalinowski commented Feb 18, 2022 •

edited

Loading

jasonmalinowski Feb 18, 2022

CyrusNajmabadi Feb 18, 2022

CyrusNajmabadi Feb 18, 2022

CyrusNajmabadi Feb 25, 2022

jasonmalinowski Feb 26, 2022

CyrusNajmabadi Feb 18, 2022

CyrusNajmabadi Feb 18, 2022

jasonmalinowski Feb 23, 2022

CyrusNajmabadi Feb 18, 2022

jasonmalinowski Feb 23, 2022

sharwell left a comment

sharwell Feb 18, 2022

CyrusNajmabadi Feb 25, 2022

jasonmalinowski Feb 26, 2022

jasonmalinowski Feb 26, 2022

CyrusNajmabadi Feb 25, 2022

CyrusNajmabadi Feb 25, 2022

jasonmalinowski Feb 26, 2022

jasonmalinowski Feb 26, 2022

CyrusNajmabadi Feb 25, 2022

CyrusNajmabadi Feb 25, 2022

CyrusNajmabadi Feb 25, 2022

jasonmalinowski Feb 26, 2022

genlu commented Mar 3, 2022

jasonmalinowski commented Mar 3, 2022

Fix more lock contention #59638

Fix more lock contention #59638

Conversation

jasonmalinowski commented Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu commented Mar 3, 2022

jasonmalinowski commented Mar 3, 2022

jasonmalinowski commented Feb 18, 2022 •

edited

Loading