Implement an LspWorkspaceManager and modify how we fork documents in LSP. by dibarbet · Pull Request #57140 · dotnet/roslyn

dibarbet · 2021-10-14T02:19:40Z

This complexifies how we fork documents in LSP in order to hopefully simplify solution caching in LSP (as well as not throw away as much stuff each time we fork).

Currently, on every LSP request, if either the workspace or LSP text changed, we fork from the workspace's current solution with every open LSP document's text. This has become a problem for a couple reasons

All open documents and projects their in have their version changed every time we fork from the workspace. This happens even if the project and document had 0 changes. Caching based on project (dependent) versions is not easily possible in this scenario.
Every time we fork, we throw away the parsed trees for all the open documents, even if the document did not change.

The idea of this PR is to 'reverse' how we fork. Instead of forking all LSP documents when anything changes, we have an incremental LSP solution that we update based on LSP text changes. This solution is eventually consistent with the workspace.

When LSP text changes come in, we only fork the relevant document.
We listen to workspace events. When we receive an event that is not for an LSP open document, we delete our incremental LSP solution so that we can fork the workspace with all open LSP documents.

This is more complex, but has a few nice properties

LSP didChange events only cause us to update the document that changed, not all open documents.
Since we incrementally update our LSP documents, we only have to re-parse the document that changed.
Since we incrementally update our LSP documents, project versions for other open documents remain unchanged.
We are not reliant on the workspace being updated frequently (which it is not in VSCode) to do checksum diffing between LSP and the workspace.

Done

~~added a bunch more tests~~
~~manual testing in vscode ext~~
~~manual testing in vs~~
~~RPS - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5352670&view=results~~ Same regressions as we're seeing in our other insertions

Followup PRs

Modify pull diagnostics 'nothing changed' caching to use project versions instead of solution crawler events - [LSP] Switch to project version caching instead of sln crawler events pull diagnostics #57380
Use 1) to implement pull diagnostics in VSCode - proposed document pull diagnostics implementation #57518

1. Manage finding what workspace + solution + document a uri maps to. 2. Manage updating the lsp view of the world from LSP and workspace changes

… and update lspworkspaceregistrationservice to be mef exported

dibarbet · 2021-10-16T05:25:48Z

+    public ImmutableArray<Workspace> GetAllRegistrations()
+        => _registrations;
+
+    public void Register(Workspace workspace)


the old implementation of ILspWorkspaceRegistrationService ended up not being useful as I needed to have events fired when workspaces were registered.

Now vscode just implements this type telling us what their host workspace is. we have an event listener that listens for workspace creation and just registers it automatically. this also helps with registering workspaces like the metadata as source workspace that are created dynamically by us, since the vscode extension has no access to it.

CyrusNajmabadi · 2021-10-18T17:31:42Z

-
-            return solution;
+            _documentChangeTracker.UpdateTrackedDocument(uri, changedText);
+            _lspWorkspaceManager.UpdateLspDocument(uri, changedText);


given the pair of htese, is it possible that _lspWorkspaceManager should itself own the _DocumentChangeTracker?

I don't think the lspWorkspaceManager can own the IDocumentChangeTracker itself - specifically there are two different IDocumentChangeTrackers given to the context depending on if the work item can mutate or not mutate (if not, it gets a NoOp that throws on updating documents).

However, it might make sense to make the LSpWorkspaceManager implement IDocumentChangeTracker for mutating requests, so then we just have a call to _documentChangeTracker.UpdateTrackedDocument. I'll see if that is cleaner.

So just took a quick look. If I do anything here it will be in a followup. But I kind of like that these are separate. The document change tracker is specifically very simple and only cares about tracking changes to LSP text. It doesn't care about where that text belongs. So it makes sense to me to leave it as a separate component.

that works for me. thanks for looking.

davidwengier · 2021-10-21T00:26:40Z

+
+                // If there was an in progress work item that failed in the queue logic, set the result of the queue item
+                // to the exception so that it bubbles back to the caller.
+                inProgressWorkItem?.HandleQueueFailure(e);


Shouldn't we clear the inProgressWork item after its been processed? the dequeue of the next work item could wait indefinitely, so really its not the in progress work item, its the "last started work item". Also for non-mutating requests I'm not sure "in progress" makes any sense anyway, as they can overlap.

yeah I'll be slightly re-working this as part of #57221

davidwengier · 2021-10-21T00:30:20Z

+
+        _lspWorkspaceRegistrationService = lspWorkspaceRegistrationService;
+        lspWorkspaceRegistrationService.WorkspaceRegistered += OnWorkspaceRegistered;
+        // This server may have been started after workspaces were registered, we need to ensure we know about them.


It's unlikely, but there is a race here right, where OnWorkspaceRegistered could throw if it gets an event fired while this next loop is running?

yup this is true, I can move after the population - will track doing that along with this followup #57276

davidwengier

+1 to Cyrus's thoughts about the workspace manager owning the document tracking.

jasonmalinowski · 2021-10-21T22:39:07Z

+        // The lsp misc files workspace has the MiscellaneousFiles workspace kind,
+        // but we don't actually want to mark it as a registered workspace in VS since we
+        // prefer the actual MiscellaneousFilesWorkspace.
+        if (workspace is LspMiscellaneousFilesWorkspace)
+        {
+            return;
+        }


Why does this have to be in the VS specific implementation? Because we don't want it registered anywhere, right? Even other hosts?

JoeRobich · 2021-10-22T00:06:45Z

Integration test failure is known. Other jobs are passing. Merging.

jasonmalinowski · 2021-10-22T00:27:42Z

        {
            var oldSolution = this.CurrentSolution;
-            var newSolution = this.SetCurrentSolution(oldSolution.WithDocumentText(documentId, text));
+            var newSolution = this.SetCurrentSolution(solution);


Was this supposed to be something else? Odd we set it even though nothing changed?

Nope - the solution here is the solution that contains the new document (with documentId)

jasonmalinowski

It merged while in the middle of review, so some comments. Worried there might be some bugs around additional/.editorconfigs that might be bad.

jasonmalinowski · 2021-10-22T00:37:36Z

    internal partial class RequestExecutionQueue
    {
+        /// <summary>
+        /// Associates LSP document URIs with the roslyn source text containing the LSP document text.


It might be useful to revise this with a reference to didChange or something to make it clearer this is where it's being accumulated.

will be fixed with #57276

jasonmalinowski · 2021-10-22T00:42:36Z

+    private readonly ILspLogger _logger;
+    private readonly IDocumentChangeTracker _documentChangeTracker;
+    private readonly LspMiscellaneousFilesWorkspace? _lspMiscellaneousFilesWorkspace;
+    private readonly RequestTelemetryLogger _requestTelemetryLogger;
+    private readonly LspWorkspaceRegistrationService _lspWorkspaceRegistrationService;


Nit: sort this along with the constructor?

would be nice if we had a style rule + codefix - I never know what we want. will update

jasonmalinowski · 2021-10-22T00:49:28Z

+            // Ensure we have the latest lsp solutions
+            var updatedSolutions = ComputeIncrementalLspSolutions_CalledUnderLock();
+
+            var hostWorkspaceSolution = updatedSolutions.FirstOrDefault(s => s.Workspace.Kind == _hostWorkspaceKind);


It's odd to me here that we're filtering on kind directly, as opposed to just asking the LspWorkspaceRegistration service directly what the workspace is. Since it's already overloaded and giving us the host kind, why not just have that method return the workspace object directly and this just becomes SingleOrDefault(s => s.Workspace == _registrationService.GetHostWorkspace()) or something? You're still holding onto the registration service regardless.

Filtering on kind over workspace is intentional. It is very possible for a workspace to get registered later. For example, in the snappy work that Cyrus was working on, the VS workspace might get created later after other workspaces have been created and registered. Of course we could enforce the creation of the host workspace upfront, but not sure if that is necessarily better

jasonmalinowski · 2021-10-22T00:53:52Z

+            // The tracked document might not be a part of this solution.
+            if (documentIds.Any())
+            {
+                solution = solution.WithDocumentText(documentIds, text);


Is this going to support changes to .editorconfig or additional files?

jasonmalinowski · 2021-10-22T01:00:53Z

+            if (incrementalSolution == null)
+            {
+                // We have no incremental lsp solution, create a new one forked from the workspace with LSP tracked documents.
+                var newIncrementalSolution = GetSolutionWithReplacedDocuments(workspace.CurrentSolution, _documentChangeTracker.GetTrackedDocuments());


Is there a reason we're re-creating these all at once, versus just creating one the next time an LSP request comes in for a given workspace on demand?

Yes - on open we also check to see if a file exists in the registered workspaces' solution so that we know if we need to add to misc.

So rather than having two different ways of looking for documents, we just fork on open. Which I don't think will be an issue since

open isn't that frequent of an operation

the set of open documents is generally small, as well as the set of workspaces we need to fork. Generally it is 1 (VS workspace), sometimes it might be 2 (if a misc file is open).

we're going to fork almost immediately since the semantic tokens / polling requests start on open.

jasonmalinowski · 2021-10-22T01:13:19Z

+
+            // Update all the documents that have a matching uri with the new source text and store as our new incremental solution.
+            // We can have multiple documents with the same URI (e.g. linked documents).
+            var solution = GetSolutionWithReplacedDocuments(findDocumentResult.First().Project.Solution, ImmutableArray.Create((uri, newSourceText)));


What happens if more than one Workspace has a document with the same URI? There could be short windows in time where it happens...

it is possible, especially with the LSP misc files workspace. We take the first found approach (always searching lsp misc last). The documents returned here though will always be from a single workspace.

jasonmalinowski · 2021-10-22T01:15:22Z

+
+        static bool IsDocumentTrackedByLsp(DocumentId changedDocumentId, Solution newWorkspaceSolution, IDocumentChangeTracker documentChangeTracker)
+        {
+            var changedDocument = newWorkspaceSolution.GetRequiredDocument(changedDocumentId);


If changedDocumentId is an additional file or .editorconfig, won't this throw? You're calling it in those cases.

yes, this will throw. I will update this to only look at normal documents since we do not support modifications to non C#/VB/F# files yet.

good catch!

jasonmalinowski · 2021-10-22T01:15:34Z

+            _workspaceToLspSolution[workspace] = null;
+        }
+
+        static bool IsDocumentTrackedByLsp(DocumentId changedDocumentId, Solution newWorkspaceSolution, IDocumentChangeTracker documentChangeTracker)


Inline? Appears to only be called in one place?

I personally prefer it like this, makes it easier to read the intention of the code with a name, even if it is used once :)

jasonmalinowski · 2021-10-22T01:21:34Z

+    {
+        lock (_gate)
+        {
+            var updatedSolutions = ResetIncrementalLspSolutions_CalledUnderLock();


Will this reset and recompute even if we're not impacted by the file at all? i.e. should we do the if check on the next line before actually doing the reset?

As in #57140 (comment)

I think we could consider doing that if it becomes a problem. But the set of workspaces is small and in most cases will just be the host workspace, so resetting all isn't necessarily an issue

jasonmalinowski · 2021-10-22T01:25:48Z

        /// should be called from a mutating request handler. See <see cref="RequestExecutionQueue"/> for more details.
        /// </summary>
-        internal class DocumentChangeTracker : IWorkspaceService, IDocumentChangeTracker
+        internal class DocumentChangeTracker : IDocumentChangeTracker


This is now a pretty thin wrapper around a Dictionary -- do we need it? Is it easier if this just gets rolled the LspWorkspaceManager? It otherwise seems anybody has to call the open/close/update methods on both this and the manager.

(what I don't like is it's not clear to me if the request queue can call them in either order or if it must call the matching methods on one component and then the other which makes me wonder if these are really as independent as we hope they might be)

yup, will have a PR out that changes this with #57276

jasonmalinowski · 2021-10-22T01:31:37Z

+            // Trigger a fork of all workspaces, the LSP document text may not be correct anymore and this document
+            // may have been removed / moved to a different workspace.
+            _ = ResetIncrementalLspSolutions_CalledUnderLock();


Similar to other comment: should this only reset impacted documents?

Implement an LspWorkspaceManager to

e015f38

1. Manage finding what workspace + solution + document a uri maps to. 2. Manage updating the lsp view of the world from LSP and workspace changes

ghost added the Area-IDE label Oct 14, 2021

runfoapp bot mentioned this pull request Oct 15, 2021

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

Open

Add a tests

bfb5546

dibarbet force-pushed the lsp_workspace_manager branch from 7ee3f99 to bfb5546 Compare October 16, 2021 02:57

dibarbet commented Oct 16, 2021

View reviewed changes

Comment thread src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs

dibarbet force-pushed the lsp_workspace_manager branch 2 times, most recently from affac5c to 135a766 Compare October 16, 2021 05:21

Make the lsp misc files workspace a normally registered lsp workspace…

02e9302

… and update lspworkspaceregistrationservice to be mef exported

dibarbet force-pushed the lsp_workspace_manager branch from 135a766 to 02e9302 Compare October 16, 2021 05:24

dibarbet commented Oct 16, 2021

View reviewed changes

Comment thread src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs Outdated