Ensure current OOP calls for the same solution-checksum can share the same OOP solution computation by CyrusNajmabadi · Pull Request #60575 · dotnet/roslyn

CyrusNajmabadi · 2022-04-05T00:21:07Z

Sending out review for initial thoughts/feedback.

The general idea here is to setup our host/OOP communication to avoid teh following problem.

user makes edit on host, moving solution from Vn to Vn+1.
immediately K features light up and call over to OOP to do work.
all features see that OOP has not sync'ed solution Vn+1 over and do the work to do just that, producing solution snapshots that may not share some state (for example the compilation-trackers for the last edited project.

To avoid this, this PR introduces a slightly different model for host-OOP calls. Now, when the host calls into OOP for a paticular feature on a particular snapshot checksum, then OOP side creates a mapping from that checksum to the async computation for taht solution (an AsyncLazy), and it keeps it alive as long as that call is in flight. If any other calls come in during that time and need that same solution, they will be given the same solution computation object back so that everything is shared across those calls. This is done in a ref-counting fashion to ensure that as long as something is calling into oop that is working on that snapshot, the lazy for it will be kept alive and any concurrent calls will just get that and do no more extra work.

We still also keep track of hte last primary-workspace-solution, and the last-solution queried for, so that if a call gets either, and then returns, thne the next call can see those and potentially benefit from it.

… same OOP solution computation

src/Workspaces/Remote/ServiceHub/Services/DocumentHighlights/RemoteDocumentHighlightsService.cs

…emoteDocumentHighlightsService.cs

...ces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.cs

…on/RemoteSemanticClassificationService.cs

…roslyn into cacheComputation

CyrusNajmabadi · 2022-04-05T00:59:03Z

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

+        /// classification).  As long as we're holding onto it, concurrent feature requests for the same checksum can
+        /// share the computation of that particular solution and avoid duplicated concurrent work.
+        /// </summary>
+        private readonly Dictionary<Checksum, (int refCount, AsyncLazy<Solution> lazySolution)> _checksumToRefCountAndLazySolution = new();


this is the meat of the PR. as long as an operation is in flight that is using a particular PinnedSolutionInfo checksum, we will just hand anyone that wants the solution for it the corresponding AsyncLazy in this dictionary. The refcount is used to keep track of concurrent callers and to release the solution from this map once nothing is asking about it.

src/Workspaces/Remote/ServiceHub/Services/CodeLensReferences/RemoteCodeLensReferencesService.cs

…emoteCodeLensReferencesService.cs

CyrusNajmabadi · 2022-04-05T01:04:57Z

...vertTupleToStructCodeRefactoringProvider/RemoteConvertTupleToStructCodeRefactoringService.cs

+            return RunServiceWithSolutionAsync(solutionInfo, async solution =>
            {
-                var solution = await GetSolutionAsync(solutionInfo, cancellationToken).ConfigureAwait(false);
                var document = solution.GetDocument(documentId);


the new pattern for OOP features. Features call RunServiceWithSolutionAsync passing in their PinnedSolutionInfo they are associated with. They also pass in a callback which is given the solution that is computed once done. This allows that computation to be shared among multiple concurrent callers into oop with teh same solution checksum.

CyrusNajmabadi · 2022-04-05T01:07:44Z

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

+                        // solution instance).
+                        return this.CurrentSolution;
+                    }
+                }


these diffs are terrible. I'm not srue why it threw it off so much to indent. working on this.

…roslyn into cacheComputation

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

sharwell · 2022-04-05T17:38:28Z

Test failure showing gold bars:

Feature 'Document highlights' is currently unavailable due to an internal error.
StreamJsonRpc.RemoteInvocationException: The given key was not present in the dictionary.
   at StreamJsonRpc.JsonRpc.<InvokeCoreAsync>d__143`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1.ConfiguredValueTaskAwaiter.GetResult()
   at Microsoft.CodeAnalysis.Remote.BrokeredServiceConnection`1.<TryInvokeAsync>d__18`1.MoveNext() in /_/src/Workspaces/Remote/Core/BrokeredServiceConnection.cs:line 217
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)
RPC server exception:
StreamJsonRpc.RemoteInvocationException: The given key was not present in the dictionary.
      at StreamJsonRpc.JsonRpc.InvokeCoreAsync[TResult](RequestId id, String targetName, IReadOnlyList`1 arguments, IReadOnlyList`1 positionalArgumentDeclaredTypes, IReadOnlyDictionary`2 namedArgumentDeclaredTypes, CancellationToken cancellationToken, Boolean isParameterObject)
      at Microsoft.CodeAnalysis.Remote.BrokeredServiceConnection`1.<>c__DisplayClass26_0`1.<<InvokeStreamingServiceAsync>b__0>d.MoveNext() in /_/src/Workspaces/Remote/Core/BrokeredServiceConnection.cs:line 402
   --- End of stack trace from previous location ---
      at Microsoft.CodeAnalysis.Remote.BrokeredServiceConnection`1.<>c__DisplayClass26_0`1.<<InvokeStreamingServiceAsync>b__0>d.MoveNext() in /_/src/Workspaces/Remote/Core/BrokeredServiceConnection.cs:line 411
   --- End of stack trace from previous location ---
      at Microsoft.CodeAnalysis.Remote.BrokeredServiceConnection`1.InvokeStreamingServiceAsync[TResult](TService service, Func`4 invocation, Func`3 reader, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/Core/BrokeredServiceConnection.cs:line 434
      at Microsoft.CodeAnalysis.Remote.RemoteCallback`1.InvokeAsync[TResult](Func`4 invocation, Func`3 reader, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/Core/RemoteCallback.cs:line 68
      at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)
   --- End of stack trace from previous location ---
      at Microsoft.CodeAnalysis.Remote.SolutionAssetSource.GetAssetsAsync(Int32 scopeId, ISet`1 checksums, ISerializerService serializerService, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Host/SolutionAssetSource.cs:line 34
      at Microsoft.CodeAnalysis.Remote.AssetProvider.RequestAssetsAsync(ISet`1 checksums, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs:line 162
      at Microsoft.CodeAnalysis.Remote.AssetProvider.RequestAssetAsync(Checksum checksum, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs:line 149
      at Microsoft.CodeAnalysis.Remote.AssetProvider.GetAssetAsync[T](Checksum checksum, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs:line 48
      at Microsoft.CodeAnalysis.Remote.RemoteWorkspace.SolutionCreator.IsIncrementalUpdateAsync(Checksum newSolutionChecksum) in /_/src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.SolutionCreator.cs:line 47
      at Microsoft.CodeAnalysis.Remote.RemoteWorkspace.ComputeSolutionAsync(AssetProvider assetProvider, Checksum solutionChecksum, Int32 workspaceVersion, Boolean fromPrimaryBranch, Solution baseSolution, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs:line 286
      at Microsoft.CodeAnalysis.Remote.RemoteWorkspace.TrySlowGetSolutionAndRunAsync[T](AssetProvider assetProvider, Checksum solutionChecksum, Int32 workspaceVersion, Boolean fromPrimaryBranch, Solution baseSolution, Func`2 doWorkAsync, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs:line 178
      at Microsoft.CodeAnalysis.Remote.RemoteWorkspace.RunWithSolutionAsync[T](AssetProvider assetProvider, Checksum solutionChecksum, Int32 workspaceVersion, Boolean fromPrimaryBranch, Func`2 doWorkAsync, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs:line 137
      at Microsoft.CodeAnalysis.Remote.BrokeredServiceBase.RunWithSolutionAsync[T](PinnedSolutionInfo solutionInfo, Func`2 doWorkAsync, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Services/BrokeredServiceBase.cs:line 91
      at Microsoft.CodeAnalysis.Remote.BrokeredServiceBase.RunServiceImplAsync[T](Func`2 implementation, CancellationToken cancellationToken) in /_/src/Workspaces/Remote/ServiceHub/Services/BrokeredServiceBase.cs:line 119

CyrusNajmabadi · 2022-04-05T20:21:10Z

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

+                    // We're not doing an update, we're moving to a new solution entirely.  Clear out the old one. This
+                    // is necessary so that we clear out any open document information this workspace is tracking. Note:
+                    // this seems suspect as the remote workspace should not be tracking any open document state.
+                    this.ClearSolutionData();


@tmat for thoughts. t his preserves what we had before... but hwat we had before seems very fishy/suspect.

Yeah, let's keep it for now and just remove the workspace updating completely once it's possible.

tmat · 2022-04-05T21:35:51Z

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

+        /// classification).  As long as we're holding onto it, concurrent feature requests for the same checksum can
+        /// share the computation of that particular solution and avoid duplicated concurrent work.
+        /// </summary>
+        private readonly Dictionary<Checksum, (int refCount, AsyncLazy<Solution> lazySolution)> _checksumToRefCountAndLazySolution = new();


_checksumToRefCountAndLazySolution

perhaps better name: _availableSolutions

Hrmm... i'm ok with the name as is. _availableSolutions is a bit too vague for me.

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspaceManager.cs

tmat · 2022-04-05T21:41:51Z

src/Workspaces/Remote/ServiceHub/Services/CodeLensReferences/RemoteCodeLensReferencesService.cs

-                    {
-                        return null;
-                    }
+                    return await RunWithSolutionAsync(solutionInfo, async solution =>


Can't we use RunServiceWithSolutionAsync?

the issue here is preserving the meaning of the using-block for telemetry.

could LogBlock be around RunServiceAsync?

let me do that in a followup.

src/Workspaces/Remote/ServiceHub/Services/BrokeredServiceBase.cs

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

tmat · 2022-04-05T21:52:16Z

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

+            {
+                using (await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
+                {
+                    if (_checksumToRefCountAndLazySolution.TryGetValue(solutionChecksum, out var tuple))


tuple

out var (refCount, lazySolution)

not a thing the language supports (yet).

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs

tmat

Great idea. Just a few suggestions to improve readability.

ghost added the Area-IDE label Apr 5, 2022

CyrusNajmabadi force-pushed the cacheComputation branch from 06974e1 to 1c43709 Compare April 5, 2022 00:22

Ensure current OOP calls for the same solution-checksum can share the…

4323e78

… same OOP solution computation

CyrusNajmabadi force-pushed the cacheComputation branch from 1c43709 to 4323e78 Compare April 5, 2022 00:22

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/DocumentHighlights/RemoteDocumentHighlightsService.cs Outdated Show resolved Hide resolved

Update src/Workspaces/Remote/ServiceHub/Services/DocumentHighlights/R…

aba0281

…emoteDocumentHighlightsService.cs

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

...ces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 5 commits April 4, 2022 17:25

Update src/Workspaces/Remote/ServiceHub/Services/SemanticClassificati…

3b96716

…on/RemoteSemanticClassificationService.cs

Revert name

3795d18

Update tests

61c78c8

Fix

c1ec0d1

Merge branch 'cacheComputation' of https://github.com/CyrusNajmabadi/…

9e31703

…roslyn into cacheComputation

CyrusNajmabadi requested review from jasonmalinowski, sharwell and tmat April 5, 2022 00:52

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

CyrusNajmabadi added 2 commits April 4, 2022 18:02

Add comment

323825a

Add comment

c905da9

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/CodeLensReferences/RemoteCodeLensReferencesService.cs Outdated Show resolved Hide resolved

Update src/Workspaces/Remote/ServiceHub/Services/CodeLensReferences/R…

6746400

…emoteCodeLensReferencesService.cs

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

fix grammar

2d29a39

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

CyrusNajmabadi added 5 commits April 4, 2022 18:08

Formatting

badc091

Merge branch 'cacheComputation' of https://github.com/CyrusNajmabadi/…

94088ba

…roslyn into cacheComputation

Revert

2866e4d

revert

f6e8009

Revert

1d94937

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs Show resolved Hide resolved

CyrusNajmabadi added 2 commits April 5, 2022 13:18

Simplify

c0df8bf

Doc

4241b4d

CyrusNajmabadi commented Apr 5, 2022

View reviewed changes

Doc

5abbc16

CyrusNajmabadi changed the title ~~WIP: Ensure current OOP calls for the same solution-checksum can share the same OOP solution computation~~ Ensure current OOP calls for the same solution-checksum can share the same OOP solution computation Apr 5, 2022

CyrusNajmabadi marked this pull request as ready for review April 5, 2022 20:23

CyrusNajmabadi requested a review from a team as a code owner April 5, 2022 20:23

tmat reviewed Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspaceManager.cs Show resolved Hide resolved

tmat reviewed Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/BrokeredServiceBase.cs Outdated Show resolved Hide resolved

tmat reviewed Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/BrokeredServiceBase.cs Outdated Show resolved Hide resolved

tmat reviewed Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Services/BrokeredServiceBase.cs Outdated Show resolved Hide resolved

tmat reviewed Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs Outdated Show resolved Hide resolved

tmat reviewed Apr 5, 2022

View reviewed changes

src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.cs Outdated Show resolved Hide resolved

tmat approved these changes Apr 5, 2022

View reviewed changes

CyrusNajmabadi added 4 commits April 5, 2022 16:16

REnames

77fb6ac

Expand tuple

62723e3

Rename parameter

aeb0853

Use deconstruction

5244b4f

CyrusNajmabadi enabled auto-merge April 5, 2022 23:42

CyrusNajmabadi merged commit eca8a13 into dotnet:main-vs-deps Apr 6, 2022

ghost added this to the Next milestone Apr 6, 2022

CyrusNajmabadi deleted the cacheComputation branch April 6, 2022 04:41

runfoapp bot mentioned this pull request Apr 6, 2022

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

CyrusNajmabadi mentioned this pull request Apr 6, 2022

Prioritize background work for visible documents #60185

Closed

7 tasks

jasonmalinowski mentioned this pull request Apr 13, 2022

ServiceHub fails to share CompilationTracker instances or otherwise synchronize dependent project parsing #59218

Closed

dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022

Conversation

CyrusNajmabadi commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sharwell commented Apr 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Uh oh!

tmat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Apr 5, 2022 •

edited

Loading