Reduce thread contention on ProjectSystemProject._gate #72424

ToddGrun · 2024-03-06T20:19:53Z

This semaphore is being flagged by thread watson here: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1881089

Modify CreateBatchScope to async so callers need not block the calling thread
Modify CreateBatchScope to take in a CancellationToken

This semaphore is being flagged by thread watson here: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1881089 1) Modify CreateBatchScope to async so callers need not block the calling thread 2) Modify CreateBatchScope to take in a CancellationToken

ToddGrun · 2024-03-06T20:43:02Z

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

@@ -282,5 +289,8 @@ public void RemoveAnalyzerConfigFile(string filePath)
            => _projectSystemProject.RemoveAnalyzerConfigFile(filePath);

        public IAsyncDisposable CreateBatchScope() => _projectSystemProject.CreateBatchScope();
+
+        public async ValueTask<IAsyncDisposable> CreateBatchScopeAsync(CancellationToken cancellationToken)


CreateBatchScopeAsync

Could CPS just use this instead of StartBatchAsync and then not need EndBatchAsync?

After talking with Drew, the assumption is that CPS can be modified to just use CreateBatchScopeAsync. I'll try to get a frankenbuild on my machine with the changes in CPS to test against this PR and will wait to elevate out of draft status until

I can at least verify the behavior based on the frankenbuild.

Both Cyrus/Drew have had a chance to look at the core idea here.

…ead utilize CreateBatchScopeAsync 2) Switch over remaining CreateBatchScope calls to CreateBatchScopeAsync inside Roslyn 3) Switch over existing (Start/End) batch calls in Roslyn and tests to CreateBatchScopeAsync

ToddGrun · 2024-03-06T21:51:48Z

Note that I am able to get a bit of data from the dmp in the cab file, it looks like there is just a long running method while holding onto the _gate. Will talk with Cyrus about this a bit but it's really orthogonal to the work in this PR.

System.Collections.Immutable.dll!System.Collections.Immutable.SortedInt32KeyNode<System.Collections.Immutable.ImmutableHashSet<System.__Canon>.HashBucket>.TryGetValue(int key, out System.Collections.Immutable.ImmutableHashSet<System.__Canon>.HashBucket value) Line 233	C#
System.Collections.Immutable.dll!System.Collections.Immutable.ImmutableHashSet<System.__Canon>.Contains(System.__Canon item, System.Collections.Immutable.ImmutableHashSet<System.__Canon>.MutationInput origin) Line 663	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.SolutionState.CreateCompilationTrackerMap(Microsoft.CodeAnalysis.ProjectId changedProjectId, Microsoft.CodeAnalysis.ProjectDependencyGraph dependencyGraph) Line 1554	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.SolutionState.ForkProject(Microsoft.CodeAnalysis.ProjectState newProjectState, Microsoft.CodeAnalysis.SolutionState.CompilationAndGeneratorDriverTranslationAction translate, Microsoft.CodeAnalysis.ProjectDependencyGraph newDependencyGraph, System.Collections.Immutable.ImmutableDictionary<string, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.DocumentId>> newFilePathToDocumentIdsMap, bool forkTracker) Line 1502	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.SolutionState.UpdateDocumentState(Microsoft.CodeAnalysis.DocumentState newDocument, bool contentChanged) Line 1450	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.SolutionState.WithDocumentContentsFrom(Microsoft.CodeAnalysis.DocumentId documentId, Microsoft.CodeAnalysis.DocumentState documentState) Line 1370	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Solution.WithDocumentContentsFrom(Microsoft.CodeAnalysis.DocumentId documentId, Microsoft.CodeAnalysis.DocumentState documentState) Line 1557	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.SetCurrentSolutionAsync.__UpdateExistingDocumentsToChangedDocumentContents|27_2(Microsoft.CodeAnalysis.Solution solution, Microsoft.CodeAnalysis.DocumentId changedDocumentId, System.Collections.Generic.HashSet<Microsoft.CodeAnalysis.DocumentId> processedDocuments) Line 331	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.SetCurrentSolutionAsync.__UnifyLinkedDocumentContents|27_0(Microsoft.CodeAnalysis.Solution oldSolution, Microsoft.CodeAnalysis.Solution newSolution) Line 301	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.SetCurrentSolutionAsync.AnonymousMethod__27_3(Microsoft.CodeAnalysis.Solution oldSolution, (Microsoft.CodeAnalysis.Workspace this, System.Func<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution> transformation, System.Action<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution> onBeforeUpdate, System.Action<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution> onAfterUpdate, System.Func<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution, (Microsoft.CodeAnalysis.WorkspaceChangeKind changeKind, Microsoft.CodeAnalysis.ProjectId projectId, Microsoft.CodeAnalysis.DocumentId documentId)> changeKind) data) Line 256	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.SetCurrentSolutionAsync<(System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon)>(bool useAsync, (System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon) data, System.Func<Microsoft.CodeAnalysis.Solution, (System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon), Microsoft.CodeAnalysis.Solution> transformation, bool mayRaiseEvents, System.Action<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution, (System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon)> onBeforeUpdate, System.Action<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution, (System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon)> onAfterUpdate, System.Threading.CancellationToken cancellationToken) Line 408	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.SetCurrentSolutionAsync(bool useAsync, System.Func<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution> transformation, System.Func<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution, (Microsoft.CodeAnalysis.WorkspaceChangeKind changeKind, Microsoft.CodeAnalysis.ProjectId projectId, Microsoft.CodeAnalysis.DocumentId documentId)> changeKind, System.Action<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution> onBeforeUpdate, System.Action<Microsoft.CodeAnalysis.Solution, Microsoft.CodeAnalysis.Solution> onAfterUpdate, System.Threading.CancellationToken cancellationToken) Line 244	C#
Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspaces.ProjectSystem.ProjectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(bool useAsync, System.Action<Microsoft.CodeAnalysis.Workspaces.ProjectSystem.SolutionChangeAccumulator> mutation) Line 257	C#

CyrusNajmabadi · 2024-03-06T22:06:39Z

src/VisualStudio/Core/Def/ProjectSystem/CPS/IWorkspaceProjectContext.cs

@@ -73,6 +74,7 @@ internal interface IWorkspaceProjectContext : IDisposable

    void StartBatch();
    IAsyncDisposable CreateBatchScope();


possibly deprecate.

Ditto for EndBatchAsync.

I added a comment in commit 3 that indicates all three methods that I think can be removed once the managed project system stops consuming them.

drewnoakes · 2024-03-06T23:35:04Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+        using (await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
+        {
+            _activeBatchScopes++;
+            return new BatchScope(this);


Looks like we could thread the cancellation token through BatchScope to its Dispose/DisposeAsync methods, where those methods call OnBatchScopeDisposedMaybeAsync (which also awaits on the gate).

Talked with Cyrus and we'll go ahead and give it a go

Actually, I think I'm going to pass on this after looking into it a bit more. Previously, the Dispose method would always fully execute, regardless of whether cancellation had occurred. I don't think that should be changed.

Fair enough. FWIW cancellation should only trigger from CPS projects when the project is being unloaded, at which point it's great to just bail immediately as everything will be torn down, but as a general mechanism it's safer to ensure a tidy disposal.

ToddGrun · 2024-03-07T03:28:30Z

Elevating out of draft status. I tried my best to get a frankenbuild to work, but I just kept hitting too many issues trying to get the managed project system code to consume a new roslyn package

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1881089 Roslyn recently introduced async API for obtaining and releasing "batch" objects for applying updates in dotnet/roslyn#72424. This change increases the package versions, uses the newer API, fixes some obsolete usages, and gets things building by adding a few package references in order to break the tie on some assembly version conflicts during build. This is another iteration of dotnet#9455, which was reverted due to issues it created during signing. Unlike that prior PR, this does not remove Microsoft.VisualStudio.Internal.MicroBuild.Swix`, so I expct the same issues about duplicated items to appear.

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 6, 2024

ToddGrun commented Mar 6, 2024

View reviewed changes

ToddGrun requested review from drewnoakes and CyrusNajmabadi March 6, 2024 21:48

CyrusNajmabadi reviewed Mar 6, 2024

View reviewed changes

CyrusNajmabadi approved these changes Mar 6, 2024

View reviewed changes

drewnoakes reviewed Mar 6, 2024

View reviewed changes

Found some more CreateBatchScope calls that could be made async

2044dff

ToddGrun marked this pull request as ready for review March 7, 2024 03:28

ToddGrun requested a review from a team as a code owner March 7, 2024 03:28

ToddGrun changed the title ~~WIP: Reduce thread contention on ProjectSystemProject._gate~~ Reduce thread contention on ProjectSystemProject._gate Mar 7, 2024

ToddGrun requested a review from CyrusNajmabadi March 7, 2024 03:30

CyrusNajmabadi approved these changes Mar 7, 2024

View reviewed changes

ToddGrun merged commit 64bc199 into dotnet:main Mar 7, 2024
27 checks passed

ToddGrun mentioned this pull request Mar 8, 2024

Re-consider usage of ProjectSystemProject._gate #72450

Open

drewnoakes mentioned this pull request Apr 29, 2024

Consume newer Roslyn APIs for language services dotnet/project-system#9455

Merged

drewnoakes mentioned this pull request May 20, 2024

Avoid contention in Roslyn language service dotnet/project-system#9471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce thread contention on ProjectSystemProject._gate #72424

Reduce thread contention on ProjectSystemProject._gate #72424

ToddGrun commented Mar 6, 2024

ToddGrun Mar 6, 2024

ToddGrun Mar 6, 2024

ToddGrun commented Mar 6, 2024 •

edited by drewnoakes

Loading

CyrusNajmabadi Mar 6, 2024

drewnoakes Mar 6, 2024

ToddGrun Mar 7, 2024 •

edited

Loading

drewnoakes Mar 6, 2024

ToddGrun Mar 6, 2024

ToddGrun Mar 7, 2024

drewnoakes Mar 7, 2024

ToddGrun commented Mar 7, 2024

		@@ -73,6 +74,7 @@ internal interface IWorkspaceProjectContext : IDisposable

		void StartBatch();
		IAsyncDisposable CreateBatchScope();

Reduce thread contention on ProjectSystemProject._gate #72424

Reduce thread contention on ProjectSystemProject._gate #72424

Conversation

ToddGrun commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun commented Mar 6, 2024 • edited by drewnoakes Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun Mar 7, 2024 • 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

ToddGrun commented Mar 7, 2024

ToddGrun commented Mar 6, 2024 •

edited by drewnoakes

Loading

ToddGrun Mar 7, 2024 •

edited

Loading