Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce overhead due to lsp semantic token refresh requests. #69690

Merged

Conversation

ToddGrun
Copy link
Contributor

Previously, typing a single character in a razor markup segment would cause 5 semantic token refresh requests, and consequently 12 semantic token range requests for the simple razor page I was testing. This PR reduces that count to 1 semantic token refresh request and 6 semantic token range requests for the same edit. It does this by:

  1. Not listening to workspace events if the language server doesn't support semantic tokens.
  2. Changing DocumentReloaded handling to detect whether something other than the text has changed on the file, and if not, prevents a refresh message if the file is being tracked.
  3. Changing AdditionalDocumentChanged to special case razor files and not send refresh notifications due to their modification.

Previously, typing a single character in a razor markup segment would cause 5 semantic token refresh requests, and consequently 12 semantic token range requests for the simple razor page I was testing. This PR reduces that count to 1 semantic token refresh request and 6 semantic token range requests for the same edit. It does this by:

1) Not listening to workspace events if the language server doesn't support semantic tokens.
2) Changing DocumentReloaded handling to detect whether something other than the text has changed on the file, and if not, prevents a refresh message if the file is being tracked.
3) Changing AdditionalDocumentChanged to special case razor files and not send refresh notifications due to their modification.
@ToddGrun ToddGrun requested a review from a team as a code owner August 23, 2023 23:07
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 23, 2023
@davidwengier
Copy link
Contributor

Rather than, or as well as, special casing Razor files, should Razor (or the LSP client) do some basic debouncing of the refresh requests? Or would that not be to spec?

@ToddGrun
Copy link
Contributor Author

Rather than, or as well as, special casing Razor files, should Razor (or the LSP client) do some basic debouncing of the refresh requests? Or would that not be to spec?

There is debouncing on both the client and server side, and yet we were still experiencing a bunch of the refresh messages and the corresponding semantic token requests.

Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than, or as well as, special casing Razor files, should Razor (or the LSP client) do some basic debouncing of the refresh requests? Or would that not be to spec?

We already debounce on the server - we send max 1 request every 2 seconds (per server). Its also entirely up to the client how it wants to handle the refresh notification. From the spec

Note that the client still has the freedom to delay the re-calculation of the semantic tokens if for example an editor is currently not visible.

private static bool DisallowsAdditionalDocumentChangedRefreshes(string? filePath)
{
var extension = Path.GetExtension(filePath);
return extension is ".cshtml" or ".razor";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some context for this -
It seems like in general additional files changing could affect semantic tokens (for example if those changes are inputs to a source generator). However, Razor semantic tokens only uses the design time file so we actually don't care of the Razor file changes (they'll already tell us when the design time file changes).

@davidwengier
Copy link
Contributor

1 request every 2 seconds and yet each keystroke would cause 5 refresh requests!? wow.

@ToddGrun
Copy link
Contributor Author

@davidwengier -- any concerns with merging?

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, didn't realise you were waiting for me :)

@ToddGrun ToddGrun merged commit 499a8d2 into dotnet:main Aug 24, 2023
24 checks passed
@ghost ghost added this to the Next milestone Aug 24, 2023
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
333fred added a commit to 333fred/roslyn that referenced this pull request Sep 5, 2023
@ToddGrun ToddGrun linked an issue Sep 12, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce overhead due to lsp semantic token refresh requests
3 participants