feat(helix-term): Don't block the UI on prompt completions - #16065
feat(helix-term): Don't block the UI on prompt completions#16065kas2020-commits wants to merge 1 commit into
Conversation
Completers for typable commands (:open, :cd, :theme, ...) performed blocking I/O on the main thread on every keystroke. Completers now return a CompletionResult, which is either an Immediate result or a Deferred closure which runs on tokio's blocking thread pool. The prompt waits up to 50ms for deferred results so fast filesystems behave exactly as before; past that deadline the results are delivered asynchronously through the job queue once ready. A TaskController cancels in-flight directory walks (checked per entry) as soon as the input changes, and swapping out the result receiver guarantees stale completions are never delivered. The unused ui::prompt_with_input helper is removed since it could not be carried over unchanged.
d9bafd3 to
8bff334
Compare
kas2020-commits
changed the title
|
CC @the-mikedavis for visibility. |
| CompletionResult::Immediate(completion) => self.completion = completion, | ||
| CompletionResult::Deferred(compute) => { | ||
| let (tx, rx) = std::sync::mpsc::sync_channel(1); | ||
| tokio::task::spawn_blocking(move || { |
There was a problem hiding this comment.
The previous implementation using AsyncHook had the property that it wouldn't spawn a new background completion until the previous one had completed. If the user is typing quickly, the new code here will spawn a background job for each keypress. The results of all these extra background jobs are ignored, but it might cause extra CPU usage or fill up tokio's thread pool.
It might make sense to go back to a strategy where there's a single background worker that only checks for a new background completion when the previous one is finished
There was a problem hiding this comment.
That's a good detail to bring up, but after some thought I'm inclined to prefer this approach a little more?
Important to note here is that the fs walk checks if the task has been canceled on every blocking call (see ui/mod:726). Once the task has been cancelled, it only has to wait for the current blocking call to complete before it completes: it doesn't do the full walk on every keystroke.
Allowing more threads to spawn in parallel brings down the overall latency at the cost of consuming more resources, so it's a balance. I expect the number of concurrent threads active at any time are still ~O(1) or maybe ~O(10) at the highest. At that magnitude, it seems like the right trade-off IMO.
wdyt?
There was a problem hiding this comment.
The fs "walk" in this case is only depth 1, so it's usually going to be a single readdir call, except in cases where the directory is really huge. So I don't expect to get much meaningful early cancellation here.
It might reduce the latency a little bit to spawn a new background completion immediately, but in practice, once the readdir call is cached by the filesystem, then all subsequent readdirs on the same directory will usually complete quickly. So even if it has to wait for a previous completion to finish, really it's waiting to get the results of that readdir into the filesystem cache which will be used by future completions.
I think it's not possible to put an upper bound on the number of concurrent threads. Imagine that I copy-paste a path into the prompt; it will spawn N background completions where N is the length of the pasted string
2 participants
Hello! I see that #11787 hasn't made progress in a while, so I thought I'd take the mantle on pushing this forward.
Credit to @rhogenson for writing the original version, which I've kept mostly the same. I made a small tweak to not use
AsyncHookthough.AsyncHook::spawncallstokio::spawn, which is intended for asyncronous tasks, whereas we're using blocking calls for I/O.tokio::task::spawn_blockingis a slightly better fit for our use-case and also lets us drop theSyncrequirement for deferred completers. There's also a few minor naming differences and comments added.I've been testing it on the google monorepo and It's been working very well for me. It would be great if we could upstream this 🙏
PS: if there's a more formal way to give attribution, please let me know!
Fixes #11604