Drop redundant document updates in the LS - #3534
Conversation
If the user types five characters, we get five document update events. If no language service requests are made in the interim, only the last one actually matters. At ~120ms each, it's worth dropping the ones we don't need. We already had code for detecting that an update superseded a previous update (same file, higher version), but it never actually kicked in because of node's event loop: processing the update blocks the event loop so keystrokes that happen during that processing get queued up in VS Code without turning into events that would queue up (and get merged) in our language service. Mine and I figured out where to put a setTimeout to yield so that more events could join the queue, but it was very fussy trying to tune the timeout duration. It also fought with a prior fix (#1682) with its own timeout. A better fix (guided by me but implemented by Copilot) makes completion wait until the version it wants is available (instead of for a fixed amount of time) and uses a flexible amount of yielding in the update handler. Benefits: - No longer sleep for >= 50ms before returning completions - Drop document updates that would serve no purpose Sadly, there's no good way to measure the improvement since we don't have the ability to detect when the keystroke comes in - we can only measure from the start of our processing to its end, skipping over the time spent waiting for a turn in the node event loop.
|
This needs both review and buddy testing since it's fairly subtle and largely Copilot's work. Particular areas to focus on:
In local testing, debug builds are actually tolerable - you have to wait 1s for a single document update, but anything that happens in the interim gets merged. This makes it a lot easier to see the merging behavior in action. |
|
Claude's summary of its own work: Adaptive yieldinglet mut updates = vec![update];
for _ in 0..MAX_YIELDS_PER_BATCH {
yield_to_host().await;
let batch_before = updates.len();
let drained = self.drain_pending(&mut updates);
if drained == 0 {
break;
}
let dropped = drained - (updates.len() - batch_before);
trace!("drained {drained} update(s), merging dropped {dropped} as redundant");
}
self.apply(updates).await;The loop asks the host "got anything else?" and stops as soon as the answer is no. Why not a fixed delay. A fixed Termination is structural, not timed. When you pause, the first yield comes back with an empty channel and the loop exits after ~1 tick. While you're typing fast, the backlog drains in one or two passes and then the next keystroke is still ~100ms away, so the following yield comes back empty. The cap of 4 only bounds a pathological case; normally it exits earlier. No clock is involved, which is why the unit test is deterministic. One subtlety: Node vs. browserThe yield has to be a macrotask. The channel wake from But not all macrotasks are equal. Node runs phases in a fixed order per iteration:
Browser (web extension host / worker) has no
Hence the runtime pick in Why inject it from TypeScript rather than detect in Rust: the host knows its own environment, and it means the primitive can be swapped or tuned without a wasm rebuild — which matters given a release wasm build is ~60s. Measured: 20 rapid edits under a 100ms simulated compile → 1 compilation on Node, 2 on web. The web host taking one extra pass is consistent with |
|
There's a hypothetical problem where you ask for completion at version N and N gets merged into N+1. My expectation was that the next update would cancel the completion session anyway, so this (presumably rare) problem was acceptable. If we find we're hitting it in practice, the solution is probably to communicate to the queue that a particular update has a caller waiting on it and is unmergeable. The current mitigation is to mark results for affected requests as incomplete so VS Code will re-query. Claude adds: Two things worth remembering: The whole design rests on that VS Code contract, so completion-retrigger.test.ts exists purely to guard it. It asserts both directions — complete lists are not re-requested, incomplete ones are. If VS Code ever changed this, that test fails loudly instead of the feature silently degrading into missing completion lists. Returning a bare array from provideCompletionItems (the original behavior) means "complete", so the vscode.CompletionList wrapper is load-bearing, not cosmetic. Same for the 2s timeout path — it also returns isIncomplete: true, so a genuinely stuck update degrades to "ask again" rather than "no completions ever". |
There was a problem hiding this comment.
Pull request overview
This PR improves Q# language service responsiveness by coalescing redundant document updates (so intermediate versions aren’t compiled) and by making completion requests wait for the exact document version they were computed against, rather than sleeping for a fixed delay. It also adds test hooks to reliably reproduce “typing while compile blocks the host” scenarios and verifies the new behavior with VS Code integration tests and Rust unit tests.
Changes:
- Add a host-yield mechanism to the update loop so queued edit events can be drained and merged before compiling.
- Change completion requests to wait for the requested document version (or detect it was superseded/timeout) and propagate
isIncompleteto control VS Code re-triggering. - Add test-only simulated compile delay plumbing plus new VS Code and Rust tests for update coalescing and completion re-trigger behavior.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| source/wasm/src/language_service.rs | Exposes host-yielded update loop and a wait_for_document_version API to JS/WASM; wires in simulated compile delay callback. |
| source/vscode/test/suites/language-service/update-coalescing.test.ts | New VS Code integration test asserting many rapid edits coalesce into fewer compilations. |
| source/vscode/test/suites/language-service/index.node.ts | Registers the new language-service test suites for Node runs. |
| source/vscode/test/suites/language-service/index.browser.ts | Registers the new language-service test suites for browser runs. |
| source/vscode/test/suites/language-service/completion-retrigger.test.ts | New test characterizing VS Code completion re-trigger behavior for complete vs incomplete lists. |
| source/vscode/src/language-service/completion.ts | Passes document version into completions call and preserves isIncomplete via vscode.CompletionList. |
| source/vscode/src/language-service/activate.ts | Plumbs simulated compile delay config into language service configuration updates. |
| source/vscode/src/config.ts | Adds accessor for dev.simulatedCompileDelayMs. |
| source/vscode/package.json | Adds hidden Q#.dev.simulatedCompileDelayMs setting for internal testing. |
| source/playground/src/main.tsx | Passes Monaco model version into completion requests. |
| source/npm/qsharp/test/languageService.js | Makes diagnostic-related test deterministic by awaiting the next diagnostics event rather than setTimeout(0). |
| source/npm/qsharp/src/language-service/language-service.ts | Introduces version-aware completions, host-yield abstraction, and completion wait timeout logic. |
| source/npm/qsharp/src/compiler/compiler.ts | Passes host-yield function to the update loop and updates shutdown/yield commentary. |
| source/language_service/src/typing_simulation.rs | Adds test-only hook for synchronous “slow compilation” simulation via a WASM-provided busy-wait callback. |
| source/language_service/src/tests.rs | Adds unit tests verifying update coalescing behavior and merge semantics. |
| source/language_service/src/state.rs | Applies simulated compile delay during compilation and adds document version access for version waiting. |
| source/language_service/src/protocol.rs | Adds simulated_compile_delay_ms to workspace configuration protocol. |
| source/language_service/src/lib.rs | Implements version waiting, update-loop yielding/coalescing, and waiter wakeup after applied batches. |
One looks like a typo. The other is a more interesting shutdown cleanup fix. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
We were requiring an exact match and returning an empty list, which would result in an empty dialog and possibly even flickering (if it had previously been non-empty). Since, prior to this change, we weren't even checking the version and since there's no clear value to return in an error state, just return what we used to - the value computed from the "future" version. Given that I can't seem to get this to happen in practice, I think it's preferable to introducing a ton of code to make it wake up at the exact revision. On top of the at, the editor will likely discard the results if a non-trivial edit has happened in the interim.
|
|
||
| if (status !== "ready") { | ||
| log.info( | ||
| `Providing completions for ${documentUri} from a ${status === "timeout" ? "older" : "newer"} version than requested`, |
There was a problem hiding this comment.
I don't love this, but it's what we would have done before this change and it seems to be rare in practice. In addition, I suspect the editor drops the results as stale.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/language_service/src/lib.rs:147
VersionWaiters::shut_downpermanently setsis_shut_down = true, andLanguageService::create_update_handlerreuses the sameRc<VersionWaiters>(version_waiters: self.version_waiters.clone()). If the update loop is ever stopped and then started again on the sameLanguageServiceinstance,wait_for_document_versionwill be unable to park (it will immediately returnSuperseded) even though the new handler is running. Consider resettingversion_waiterswhen creating a new handler after shutdown so version waits still work after a restart.
recv,
version_waiters: self.version_waiters.clone(),
};
If the user types five characters, we get five document update events. If no language service requests are made in the interim, only the last one actually matters. At ~120ms each, it's worth dropping the ones we don't need.
We already had code for detecting that an update superseded a previous update (same file, higher version), but it never actually kicked in because of node's event loop: processing the update blocks the event loop so keystrokes that happen during that processing get queued up in VS Code without turning into events that would queue up (and get merged) in our language service.
Mine and I figured out where to put a setTimeout to yield so that more events could join the queue, but it was very fussy trying to tune the timeout duration. It also fought with a prior fix (#1682) with its own timeout.
A better fix (guided by me but implemented by Copilot) makes completion wait until the version it wants is available (instead of for a fixed amount of time) and uses a flexible amount of yielding in the update handler.
Benefits:
Sadly, there's no good way to measure the improvement since we don't have the ability to detect when the keystroke comes in - we can only measure from the start of our processing to its end, skipping over the time spent waiting for a turn in the node event loop.