Add support for LSP semantic token refresh. #61003

CyrusNajmabadi · 2022-04-27T23:32:53Z

This PR gets rid of the IsFinalized bit on semantic tokens and replaces it with the new server-initialized notification that information may be changed and semantic tokens should be 'refreshed'.

The impl here works similarly to what roslyn already does in semantic tagging. Namely, we attempt to service the tagging request using a 'frozen partial' view of hte world (for fast latency). Then, we also issue a request to our OOP server to ensure it is fully up to date with teh compilation for the corresponding project. Once that compilation is ready on the OOP side, we notify teh tagger that it should retag.

Things are slightly more complicated in the LSP side as we don't have a dedicated party asking for tags and a dedicated party notifying us of changes. Instead, it's the same party. e.g. we have to use the presence of a request to get semantic tokens as our way of knowing to kick off the compilation computation work in the OOP process. This comes with a potential downside of running into the following infinite loop:

client calls into roslyn LSP to get semantic tokens.
roslyn LSP computes fast results and returns them, while also kicking off work to get compilation.
after compilation is produced, we issue a semantic tokens refresh notification. this causes:
client calls into roslyn LSP to get semantic tokens (e.g. step1, and this cycles endlessly).

To avoid this, we insert the following fixed point break in the above code:

after compilation is produced, we compute and attempt to store the checksum of hte solution we computed against. If this checksum is the same as the last time we did this, we stop processing. If it is different, then we issue a semantic tokens refresh notification.

This means that when the client calls in in response to the semantic tokens refresh, we will attempt to make the compilation. However, we will see that we produced it for the same solution checksum, and decide to not notify the client to refresh a second time, ending hte loop.

CyrusNajmabadi · 2022-04-27T23:33:25Z

src/EditorFeatures/Core/Tagging/CompilationAvailableTaggerEventSource.cs

-        /// Cancellation tokens controlling background computation of the compilation.
-        /// </summary>
-        private readonly ReferenceCountedDisposable<CancellationSeries> _cancellationSeries = new(new CancellationSeries());
+        private readonly CompilationAvailableEventSource _eventSource;


common code extracted into this new type.

CyrusNajmabadi · 2022-04-27T23:36:35Z

src/Features/LanguageServer/Protocol/Handler/RequestContext.cs

@@ -59,6 +60,10 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler

        public readonly IGlobalOptionService GlobalOptions;

+        public readonly LspWorkspaceManager LspWorkspaceManager;
+        public readonly ILanguageServerNotificationManager NotificationManager;
+        public readonly CancellationToken QueueCancellationToken;


ew.

#61002 tracks a nicer way to export/discover LSP services so we don't have to pass all this around in this RequestCOntext.

src/Features/LanguageServer/Protocol/Handler/RequestContext.cs

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

dibarbet · 2022-04-27T23:39:36Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

            {
-                if (isFullyLoaded && !_projectIdToCompilation.ContainsKey(project.Id))
+                if (_projectIdLastComputedSolutionChecksum.TryGetValue(project.Id, out var lastComputedChecksum) &&


I think this needs to be moved up - this looks like we compute the compilation then afterwards we compare to the last result. But that means when the checksum is the same we compute the compilation anyway, we just don't store it or send a notification

dibarbet · 2022-04-27T23:40:15Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

+        {
+            lock (_gate)
+            {
+                if (_supportsRefresh == null)


I apologize for this mess :(
Evidence for #61002

dibarbet · 2022-04-27T23:42:09Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs


        public override bool MutatesSolutionState => false;
        public override bool RequiresLSPSolution => true;

+        private readonly object _gate = new();
+        private readonly Dictionary<ProjectId, CompilationAvailableEventSource> _projectIdToEventSource = new();


could this be a pair of last checksum and compilationavailable event source in one map?

CyrusNajmabadi · 2022-04-28T00:15:14Z

@allisonchou @dibarbet @ryanbrandenburg @NTaylorMullen Please try this out at your soonest convenience to see how well this works (both in terms of correctness and in terms of performance).

Note: that you should expect a potential long delay before a semantic change happening, and seeing the refresh for another file. Right now there is a hardcoded 3s delay on us producing compilations in OOP (due to cost) that can be very observable if people are waiting on it. However, the hope is that in practice this delay is good for keeping utilization low, while ensuring that in the BG as people are doing other things that we're bringing things up to day.

CyrusNajmabadi · 2022-04-28T00:23:13Z

Note: #60993 was merged into this PR.

allisonchou

LGTM! I manually ran a VS instance with these changes and colorization overall looks good. I noticed minor lag when typing but that's also present without the changes.

In terms of what changes with Cyrus' modifications, I didn't notice a difference even in larger files. However, it's probable that this bug manifests itself occasionally (+ my testing likely wasn't comprehensive), so I think it'd still be good to take this through M2 if possible.

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

allisonchou · 2022-04-28T07:20:24Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

        {
            _globalOptions = globalOptions;
+            _asyncListener = asynchronousOperationListenerProvider.GetListener(FeatureAttribute.Classification);


question: In the prior implementation I passed in FeatureAttribute.Workspace here, was that a bug/what implications does passing in FeatureAttribute.Classification vs FeatureAttribute.Workspace have?

Great question. The general idea of hte "Async Listener" system is to allow features to let an external system know "i'm doing async work, so if you need information from me, you should wait until that work is done". This system exists only for test purposes so that tests can simulate what the user is doing, and then check for the expected results as soon as possible (e.g. instead of polling and then timing out once a certain amount of time has gone past).

The 'feature attribute' is jsut a broad way of saying what sort of functionality the async work is being done for. that way the test can just way for that general area to be done, as opposed to waiting for async work anywhere (as there may be lots of async work happening that the test doesn't care about at all).

So here, we want to use 'Classification' so that if we have a test of LSP classification, and it only wanted to run once there was no bg-classification work, it would have a way to do it.

In general, using 'Workspace' is not a great idea as it's a very 'catch all bucket' that can mean the test has to wait a long time for everything in the workspace to settle down.

Thanks! That was a very helpful explanation

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

allisonchou · 2022-04-28T07:31:06Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

+                // to completion) prior to the next request running and completing.  In practice this should not happen
+                // as cancellation is checked fairly regularly.  However, if it does, check and do not bother to issue a
+                // refresh in this case.
+                if (ChecksumIsUnchanged_NoLock(project, projectChecksum))


Maybe just making this a local function and calling it something like IsChecksumUnchanged would be the most concise? I feel like the naming of ChecksumIsUnchanged_NoLock is confusing without context. I'm guessing it just means that the method's implementation isn't protected by a lock, but without context and just looking at the name, I feel like it could also be interpreted as that the method shouldn't be called under a lock

allisonchou · 2022-04-28T07:38:57Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

-            // for isFinalized. It may not be completely accurate but this is only a a temporary fix until
-            // workspace/semanticTokens/refresh is implemented.
-            lock (_lock)
+            // Determine the checksum for this project cone.  Note: this should be fast in practice because this is


question: what's a project "cone"? Is it just another way to say project version?

i use "cone" to mean "this project and the projects it depends on". "cones" are useful for features that need semantics, but couldn't be affected by any projects outside of htat set. For example if i have projects A<-B<-C then to get classification in 'B', i only need the information in 'A' and 'B' as the information in C cannot affect it. So when we're sync'ing to oop, we actually say "here's the checksum for A+B (but not C), so just sync that information over".

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs

CyrusNajmabadi · 2022-04-28T17:33:10Z

In terms of what changes with Cyrus' modifications, I didn't notice a difference even in larger files. However, it's probable that this bug manifests itself occasionally (+ my testing likely wasn't comprehensive),

@allisonchou It would be good to know if you're seeing a reasonable set of LSP messages happening. E.g. that you do see the eventual call to 'refresh' and that that causes a callback that does not cause another refresh, etc. etc.

so I think it'd still be good to take this through M2 if possible.

If we're not really noticing any difference, my strong preference would be to take this in P2 instead of through P1. There's no need to take on risk here IMO if we don't have a substantive problem and this PR isn't making massive improvements.

allisonchou · 2022-04-28T19:31:39Z

@CyrusNajmabadi I used the VS telemetry monitor and it appears that we're getting the refresh notifications properly. I had 5 projects open and saw exactly 5 refresh notifications after adding a file. Didn't notice any cycling:

As for delaying this PR til preview 2, if you think the risk factor is high then that would be OK with me. @ryanbrandenburg what do you think? My only concern is that my manual testing could have missed something and that this PR would solve any edge cases.

CyrusNajmabadi · 2022-04-28T19:40:31Z

@CyrusNajmabadi I used the VS telemetry monitor and it appears that we're getting the refresh notifications properly. I had 5 projects open and saw exactly 5 refresh notifications after adding a file. Didn't notice any cycling:

Awesome. you rock!

CyrusNajmabadi · 2022-04-28T19:41:31Z

As for delaying this PR til preview 2, if you think the risk factor is high then that would be OK with me. @ryanbrandenburg what do you think? My only concern is that my manual testing could have missed something and that this PR would solve any edge cases.

My main concern is that if we don't really see a problem with our current code, and we're not seeing much of a change, then i think it's a bit hard to justify the risk. We have plenty of runway in front of us i think (though @jinujoseph to weigh in), so we can put this in P2 and get lots of time to preview/dogfood/etc.

jinujoseph · 2022-04-28T19:48:18Z

I agree , lets not rush the churn in preview1 ...we can take this for preview2 ( feel free to retarget to main )

CyrusNajmabadi · 2022-04-28T19:50:20Z

Retargetting to main.

* Allow non-Roslyn languages to use Formatter API * Custom document options provider * Fix typo

…manticTokensRangeHandler.cs Co-authored-by: Allison Chou <[email protected]>

CyrusNajmabadi requested review from NTaylorMullen, dibarbet, ryanbrandenburg and allisonchou April 27, 2022 23:32

CyrusNajmabadi requested a review from a team as a code owner April 27, 2022 23:32

dotnet-issue-labeler bot added the Area-IDE label Apr 27, 2022

CyrusNajmabadi commented Apr 27, 2022

View reviewed changes

dibarbet reviewed Apr 27, 2022

View reviewed changes

dibarbet approved these changes Apr 28, 2022

View reviewed changes

allisonchou approved these changes Apr 28, 2022

View reviewed changes

CyrusNajmabadi changed the base branch from release/dev17.3 to main April 28, 2022 19:49

CyrusNajmabadi and others added 10 commits April 28, 2022 12:52

Allow non-Roslyn languages to use Formatter API (dotnet#60971)

f0fbe35

* Allow non-Roslyn languages to use Formatter API * Custom document options provider * Fix typo

Extract out common code to shared locatio

a59c920

Remove IsFinalized logic

c1894ca

Simplify

d4d00e4

thread through data

5559ab7

Add docs

a637788

Add docs

f68b243

Cleanup

5a99a75

Simplify

ef9c2f7

Simplify and add comment

98e12d6

CyrusNajmabadi and others added 8 commits April 28, 2022 12:52

Change delay

27236fc

Fix signature

23a291e

remove unnecessary code

7216858

Add comment

f053004

Simplify

fd2c499

Feedback

8f44048

Add region

778a821

Update src/Features/LanguageServer/Protocol/Handler/SemanticTokens/Se…

157b3de

…manticTokensRangeHandler.cs Co-authored-by: Allison Chou <[email protected]>

CyrusNajmabadi force-pushed the lspRefresh branch from 85b4c3f to 157b3de Compare April 28, 2022 19:52

REvert

55dea73

CyrusNajmabadi enabled auto-merge April 28, 2022 19:56

allisonchou mentioned this pull request Apr 28, 2022

[LSP] Send semantic tokens refresh notifications when compilations are available #60991

Closed

CyrusNajmabadi merged commit f4cc5c0 into dotnet:main Apr 28, 2022

ghost added this to the Next milestone Apr 28, 2022

CyrusNajmabadi deleted the lspRefresh branch April 29, 2022 21:11

Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for LSP semantic token refresh. #61003

Add support for LSP semantic token refresh. #61003

CyrusNajmabadi commented Apr 27, 2022

CyrusNajmabadi Apr 27, 2022

CyrusNajmabadi Apr 27, 2022 •

edited

Loading

dibarbet Apr 27, 2022

dibarbet Apr 27, 2022

dibarbet Apr 27, 2022

CyrusNajmabadi commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

allisonchou left a comment

allisonchou Apr 28, 2022

CyrusNajmabadi Apr 28, 2022

allisonchou Apr 28, 2022

allisonchou Apr 28, 2022

allisonchou Apr 28, 2022

CyrusNajmabadi Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022 •

edited

Loading

allisonchou commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

jinujoseph commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

Add support for LSP semantic token refresh. #61003

Add support for LSP semantic token refresh. #61003

Conversation

CyrusNajmabadi commented Apr 27, 2022

Choose a reason for hiding this comment

CyrusNajmabadi Apr 27, 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

CyrusNajmabadi commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

allisonchou 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

CyrusNajmabadi commented Apr 28, 2022 • edited Loading

allisonchou commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

jinujoseph commented Apr 28, 2022

CyrusNajmabadi commented Apr 28, 2022

CyrusNajmabadi Apr 27, 2022 •

edited

Loading

CyrusNajmabadi commented Apr 28, 2022 •

edited

Loading