[LSP] Send semantic tokens refresh notifications when compilations are available #60991
Conversation
allisonchou
added
the
LSP
allisonchou
requested review from
davidwengier,
NTaylorMullen,
CyrusNajmabadi,
dibarbet and
ryanbrandenburg
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| await project.GetCompilationAsync(cancellationToken).ConfigureAwait(false); | ||
| await _languageServerNotificationManager.SendNotificationAsync( |
There was a problem hiding this comment.
Razor debounces refresh notifications so sending more notifications shouldn't be an issue. @gundermanc just to confirm, does the LSP client do the same?
There was a problem hiding this comment.
@gundermanc just to confirm, does the LSP client do the same?
I don't think we do that currently.
There was a problem hiding this comment.
Would debouncing on the client be an option for M2? Maybe batching all the requests within a 1-2 second window?
There was a problem hiding this comment.
I think we should de-bounce on our side regardless of what the client does, either with feeding this as an event from range (as my other comments) or via queuing this compilation available event into the work queue
There was a problem hiding this comment.
Would debouncing on the client be an option for M2? Maybe batching all the requests within a 1-2 second window?
I would vote we not take that to the Client for M2 to avoid churn. It's an optimization rather than a "bug fix" (unless I'm missing something) and I would project that while the impact would be measurable it probably wouldn't cause high-level Watsons or anything (famous last words).
allisonchou
changed the title
| continue; | ||
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| await project.GetCompilationAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
This definitely needs to check capabilities - we don't want to do this if the semantic tokens capabilities are not on
| { | ||
| // Currently, we send a refresh notification to the client whenever there are any workspace changes. | ||
| // However, we also need to send notifications whenever the compilations finish processing since | ||
| // compilations impact semantic tokens. We put this logic here rather than in the semantic tokens |
There was a problem hiding this comment.
I wonder - can this still go into the range handler itself? Basically when we get a semantic tokens range request we trigger the compilation request (if not available) which triggers an event of some kind that feeds here?
That way we get de-bouncing as well on our side.
| var projects = trackedDocuments.Select( | ||
| d => _lspWorkspaceManager.GetLspDocument(new TextDocumentIdentifier { Uri = d.Key })?.Project).Distinct(); | ||
|
|
||
| foreach (var project in projects) |
There was a problem hiding this comment.
This is potentially problematic as we'll be trying to get a lot more compilations because we're now calculating compilations for all changes to all the open projects instead of just the snapshot of the project that is being requested by the semantic tokens range handling.
I think we definitely only want to trigger the compilation for the project requested by the range handler.
There was a problem hiding this comment.
I think we definitely only want to trigger the compilation for the project requested by the range handler.
This is my thinking too, but I'd like to hear the thought process to build all projects in case I'm missing something.
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| await project.GetCompilationAsync(cancellationToken).ConfigureAwait(false); | ||
| await _languageServerNotificationManager.SendNotificationAsync( |
There was a problem hiding this comment.
I think we should de-bounce on our side regardless of what the client does, either with feeding this as an event from range (as my other comments) or via queuing this compilation available event into the work queue
|
Superseded by #61003 |
Currently, we only send semantic tokens refresh notifications on LSP workspace changes. However, after discussing with Taylor/Ryan/Cyrus, we should also be sending refresh notifications when the compilations for each project are available since compilations affect semantic tokens.