-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
perf(lsp): cancellable TS diagnostics #13565
Conversation
@@ -217,6 +217,7 @@ impl DiagnosticsServer { | |||
snapshot.clone(), | |||
&config, | |||
&ts_server, | |||
token.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change we might want to consider removing or reducing the 200ms delay on line 208.
Edit: I'm just trying out the big file benchmark to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also antidotal evidence too by using a debug build and editing a large project in vscode... I can try to take it for a spin later... though if we drop that, we should do it as a follow up PR, I suspect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea! Though I just tested it now and removing the delay is still much slower ☹
Before (this PR):
Starting Deno benchmark
-> Start benchmarking lsp
- Simple Startup/Shutdown
(10 runs, mean: 508ms)
- Big Document/Several Edits
(5 runs, mean: 1879ms)
- Find/Replace
(10 runs, mean: 78ms)
- Code Lens
(10 runs, mean: 612ms)
<- End benchmarking lsp
After (debounce removed):
Starting Deno benchmark
-> Start benchmarking lsp
- Simple Startup/Shutdown
(10 runs, mean: 295ms)
- Big Document/Several Edits
(5 runs, mean: 4668ms)
- Find/Replace
(10 runs, mean: 78ms)
- Code Lens
(10 runs, mean: 402ms)
<- End benchmarking lsp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test might not be triggering it, because I don't think it is sending and setting cancellation tokens. I think we would need to modify the test, based on observed messages to and from the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM implementation wise, but I'm unsure about the debouncing. I'll leave that to Kit
Adds the ability to cancel generating TypeScript diagnostics while they're being generated. I'm not sure how to test this to see what kind of improvement this may bring, but I assume it will be helpful.