perf(lsp): avoid blocking on diagnostic requests#23768
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
8bbea75 to
de5ca03
Compare
github-actions
Bot
added
A-linter
4780095 to
ee1f6aa
Compare
de5ca03 to
cc897b0
Compare
cc897b0 to
111bdc5
Compare
111bdc5 to
0fb54e2
Compare
|
I've been running this stack in IntelliJ on a large monorepo and it seems to help a lot — the wait before Fix-All (where it used to sit behind other runs for a few seconds) is basically gone in my testing, and stale runs get dropped as expected. Thanks for putting it together. One small follow-up I noticed on top: with diagnostics running off-thread, a single save seems to kick off two full type-aware lints for the same Would you be up for pushing this stack forward — marking #23768 / #23797 ready and pinging a maintainer to get them reviewed and landed? They've been sitting for a few weeks, and they'd unblock a real, measurable win for the JetBrains users (oxc-project/oxc-intellij-plugin#366). |
|
Thank you, @evilsamaritan! I will look into your improvements, sadly this PR is not ready yet. I did not have time to finish it up. + I found one big problem working on oxc-project/eslint-plugin-oxlint#741. Somehow a code action request will not receive a response. |
There was a problem hiding this comment.
Pull request overview
This PR updates the oxc_language_server LSP implementation to prevent slow/CPU-heavy diagnostics work from blocking unrelated requests by moving diagnostic execution into background blocking tasks and making the involved data/worker handles safe to move across tasks.
Changes:
- Move diagnostic execution (
didOpen/didChange/didSaveand pull diagnostics request) intotokio::task::spawn_blockingto avoid blocking the async runtime. - Change
TextDocumentto own itsUrito allow safe'staticmoves into spawned tasks. - Store
WorkspaceWorkerinstances behindArcinWorkerManagerand return cloned handles instead of lock-tied guards.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_language_server/src/worker.rs | Adjust TextDocument references and file-system document retrieval to work with owned URIs. |
| crates/oxc_language_server/src/worker_manager.rs | Store workers as Arc<WorkspaceWorker> and return cloned worker handles for task-safe usage. |
| crates/oxc_language_server/src/tests.rs | Re-enable the request-lock test (removes #[ignore]). |
| crates/oxc_language_server/src/lib.rs | Make TextDocument own Uri (removes lifetime parameter). |
| crates/oxc_language_server/src/file_system.rs | Update get_document to return an owned TextDocument by taking Uri by value. |
| crates/oxc_language_server/src/backend.rs | Spawn diagnostics on blocking tasks and adapt call sites to Arc workers / owned TextDocument. |
| apps/oxlint/src/lsp/tester.rs | Update TextDocument::new call sites for owned Uri. |
| apps/oxlint/src/lsp/server_linter.rs | Pass &document.uri where APIs now expect &Uri. |
| apps/oxfmt/src/lsp/server_formatter.rs | Pass &document.uri where APIs now expect &Uri. |
| // use `spawn_blocking` because this is a very CPU intensive task and we do not want to block the async runtime. | ||
| let diagnostics = tokio::task::spawn_blocking(move || { | ||
| handle.block_on(async move { worker.run_diagnostic_on_save(&document).await }) | ||
| }) | ||
| .await | ||
| // unwrap because we want to panic if the task panics, | ||
| // because it should not happen and we want to know about it. | ||
| .unwrap(); | ||
|
|
| // use `spawn_blocking` because this is a very CPU intensive task and we do not want to block the async runtime. | ||
| let diagnostics = tokio::task::spawn_blocking(move || { | ||
| handle.block_on(async move { worker.run_diagnostic_on_change(&document).await }) | ||
| }) | ||
| .await | ||
| // unwrap because we want to panic if the task panics, | ||
| // because it should not happen and we want to know about it. | ||
| .unwrap(); | ||
|
|
| // use `spawn_blocking` because this is a very CPU intensive task and we do not want to block the async runtime. | ||
| let diagnostics = tokio::task::spawn_blocking(move || { | ||
| handle.block_on(async move { worker.run_diagnostic(&document).await }) | ||
| }) | ||
| .await | ||
| // unwrap because we want to panic if the task panics, | ||
| // because it should not happen and we want to know about it. | ||
| .unwrap(); | ||
|
|
| // use `spawn_blocking` because this is a very CPU intensive task and we do not want to block the async runtime. | ||
| let diagnostics = tokio::task::spawn_blocking(move || { | ||
| handle.block_on(async move { worker.run_diagnostic(&document).await }) | ||
| }) | ||
| .await | ||
| // unwrap because we want to panic if the task panics, | ||
| // because it should not happen and we want to know about it. | ||
| .unwrap(); |
| let uri_diagnostics = diagnostics | ||
| .iter() | ||
| .filter(|(diag_uri, _)| diag_uri == uri) | ||
| .filter(|(diag_uri, _)| *diag_uri == uri) | ||
| .flat_map(|(_, diags)| diags.clone()) | ||
| .collect::<Vec<_>>(); |
| let related_diagnostics = | ||
| diagnostics.into_iter().filter(|(diag_uri, _)| diag_uri != uri).collect::<Vec<_>>(); | ||
| diagnostics.into_iter().filter(|(diag_uri, _)| *diag_uri != uri).collect::<Vec<_>>(); |
…ot opened (#24676) Some clients can send code actions for documents, which are not opened and already analyzed with a lint process. A good example is the IntelliJ plugin, which allows the "fix all" code action from the file explorer. > This PR refactors the language-server → tool code-action API to pass a single request struct (including an “is document open” flag), enabling the oxlint LSP linter to avoid re-running lint for code-action requests when the document is already open (and presumably already linted/cached). This is mostly needed for #23768
This PR comes from different use cases, and I hope I did not have a wrong understanding about spawning child. ### `std::spawn` in `tokio` runtime Because the LSP is in a `tokio` runtime, we should avoid spawning threads with `std`, instead using `tokio::spawn`. I try to work already with `tokio::spawn` in the `oxc_language_server` side with #23768 So this will spawn the complete diagnostic path into one separate process. We already need to wait for the `tsgolint` result, so spawning one thread there is suboptimal(?) ### `disable_directives_map` lock for the whole thread spawn Asked Codex Terra 5.6 where a lock could be generated in #23768, and it found this long-lock. > The problematic lock scope is tsgolint.rs line 403: the stdout-reader takes disable_directives_map.lock() before iterating the entire TsGoLintMessageStream. The diagnostic reader therefore serializes all type-aware runs, including code actions. This is especially visible after introducing concurrent diagnostics. ### tsgolint process still running Some users did report that many tsgolint processes are still running, even if not typed. My first guess is these are zombie processes and can be safely killed. I did not find the exact issue but oxc-project/oxc-vscode#264 should be good enough for a reference.
Summary
TextDocumentown its URI, so diagnostic work can be moved into spawned tasks safely.Arcand return cloned worker handles fromWorkerManager.Generated by codex 5.5 - reviewed by myself.