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

perf: cancel diagnostics #2216

Merged
merged 3 commits into from
Nov 24, 2023
Merged

Conversation

jasonlyu123
Copy link
Member

Another part of #2179 is that TypeScript took a long time to type-check the file. When the file is ts or check-js, it'll be getSemanticDiagnostics, and for non-checked js, it's getSuggestionDiagnostics. There is probably not much we can do about it. But we can instead cancel the check when the file is updated. The cancellation can only be checked if there is an async operation. I added a small delay in the typescript check so that the update notification queue might run in that time frame. And we could return early for the svelte and typescript diagnostic. CSS and HTML are pure sync code and generally not that heavy, so it's probably not worth the overhead.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

We have cancellation tokens elsewhere with abort logic. What's different to this one, or rather, why can't we reuse the logic from the other cancellation?

@jasonlyu123
Copy link
Member Author

It's the same since vscode-language-server uses this as well. But because we manage the diagnostic, we need to manage it and cancel ourselves. And since diagnostics have already been debounced, adding two small delays might not make a huge difference anyway.

LSP 3.17 has a diagnostics pull model where the client manages the diagnostics. With that, it would be similar to what we have in other features. We can also try switching to the pull model in supported clients. But it probably still makes sense to add it to the existing push model so unsupported clients can benefit as well.

@dummdidumm dummdidumm merged commit 2ff3a7c into sveltejs:master Nov 24, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants