Skip to content

Drop redundant document updates in the LS - #3534

Open
Andrew Casey (amcasey) wants to merge 13 commits into
mainfrom
amcasey/MergeUpdates
Open

Drop redundant document updates in the LS#3534
Andrew Casey (amcasey) wants to merge 13 commits into
mainfrom
amcasey/MergeUpdates

Conversation

@amcasey

Copy link
Copy Markdown
Member

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.

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.
@amcasey

Copy link
Copy Markdown
Member Author

This needs both review and buddy testing since it's fairly subtle and largely Copilot's work. Particular areas to focus on:

  1. Rust memory management. I've noticed it's a little clone-happy.
  2. Testing. Probably overkill.
  3. VS Code vs Playground, since they use different yield mechanisms.

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.

@amcasey

Copy link
Copy Markdown
Member Author

Claude's summary of its own work:

Adaptive yielding

let 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 setTimeout(20) pays 20ms on every update, including a single keystroke after a long pause — pure added diagnostics latency for zero benefit. And the right value depends on typing speed, which is exactly the thing you said varies too much to tune against. The adaptive loop sidesteps the question: it's not waiting for time, it's waiting for the host to run out of queued input.

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: drained counts try_recv successes, not batch growth, because push_update merges in place. Using updates.len() would read "nothing arrived" when in fact five updates arrived and all merged — and the loop would stop absorbing exactly when it shouldn't.

Node vs. browser

The yield has to be a macrotask. The channel wake from unbounded_send is a microtask (wasm-bindgen-futures polls via promises), and microtasks drain before the host can dispatch anything — so without an explicit macrotask hop the loop resumes having seen nothing.

But not all macrotasks are equal.

Node runs phases in a fixed order per iteration:

Phase What runs
timers setTimeout / setInterval
poll I/O — the extension host reads queued IPC messages here
check setImmediate

setTimeout(0) lands in timers, which precedes poll. Schedule one and it fires at the top of the next iteration — before that iteration reads the socket. The pending keystrokes are still sitting unread. This is why the original fixed delay needed 20ms: it wasn't waiting for a duration, it was waiting long enough that a poll phase happened to occur in some intervening iteration.

setImmediate lands in check, immediately after poll in the same iteration. One hop guarantees everything currently readable has been read and dispatched. That's precisely the semantics we want, and it's why the delay could drop to zero.

Browser (web extension host / worker) has no setImmediate. Its task queue is FIFO, and host message events are tasks, so a task posted via MessageChannel queues behind the already-pending message tasks — same guarantee by a different mechanism.

setTimeout(0) is also actively worse here: per the HTML spec, once nesting depth exceeds 5 the timeout is clamped to a 4ms minimum. Our yields chain from within timer callbacks, so they'd hit that clamp. MessageChannel isn't clamped.

Hence the runtime pick in createHostYield(): setImmediateMessageChannelsetTimeout(0) as a last resort.

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 MessageChannel being a slightly coarser approximation of "after all pending input" than setImmediate's exact phase placement.

@amcasey

Copy link
Copy Markdown
Member Author

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".

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 isIncomplete to 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.

Comment thread source/language_service/src/lib.rs Outdated
Comment thread source/language_service/src/lib.rs
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>
Comment thread source/language_service/src/tests.rs
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`,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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_down permanently sets is_shut_down = true, and LanguageService::create_update_handler reuses the same Rc<VersionWaiters> (version_waiters: self.version_waiters.clone()). If the update loop is ever stopped and then started again on the same LanguageService instance, wait_for_document_version will be unable to park (it will immediately return Superseded) even though the new handler is running. Consider resetting version_waiters when creating a new handler after shutdown so version waits still work after a restart.
            recv,
            version_waiters: self.version_waiters.clone(),
        };

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.

3 participants