-
Notifications
You must be signed in to change notification settings - Fork 90
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
LSP evaluation in the background #1814
Conversation
601d275
to
35758ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for a separate subprocess vs a separate thread? Is it mostly to avoid having a panic shutting down the rest of the LSP? A subprocess offers some kind of isolation, but communication is also more costly
lsp/nls/src/background.rs
Outdated
let mut dedup = VecDeque::new(); | ||
for path in evals.iter().rev() { | ||
if seen.insert(path) { | ||
dedup.push_front(path.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: couldn't we just use a Vec
, push
and then iterate in reverse for dedup? As it seems we always push only on one side (front)
Thanks for the comments! I'll try to get another revision up later this week. The main motivation for a subprocess was to allow cancellation. I'm not too worried about panics, but I am worried about long-running evaluations. I guess another option would be a "gas"-powered evaluation, like the way equality is tested in the typechecker. |
I think that might not prove easy, at least not without baking this in the interpreter (which would be the one that needs to count the gas). At first sight, I think a timeout is better, because time is probably what we want to bound in practice.
FWIW, I found this short post of matklad, which seems to show that clean thread cancellation is not too cumbersome: https://matklad.github.io/2018/03/03/stopping-a-rust-worker.html |
I think matklad's solution doesn't work if the worker is doing a long-running computation, right? It relies on having a cooperative cancellation point (i.e. the point where the worker tries to receive it's next message). I looked a bit into terminating threads. The best I found was this crate (which uses |
You're right, I skimmed the post and thought you could just drop the thread object, but he's dropping the
Ok, maybe a process is fair then. |
Ok, I think this is worth another look. I've been test-driving it a little and decided that the false positives on incomplete configurations was too annoying, so instead I've just stopped issuing errors on missing fields. |
lsp/nls/src/background.rs
Outdated
fn eval_permissive( | ||
vm: &mut VirtualMachine<impl ImportResolver, impl Cache>, | ||
errors: &mut Vec<EvalError>, | ||
rt: RichTerm, | ||
) { | ||
match vm.eval(rt) { | ||
Err(e) => errors.push(e), | ||
Ok(t) => match t.as_ref() { | ||
Term::Array(ts, _) => { | ||
for t in ts.iter() { | ||
// After eval_closure, all the array elements are | ||
// closurized already, so we don't need to do any tracking | ||
// of the env. | ||
eval_permissive(vm, errors, t.clone()); | ||
} | ||
} | ||
Term::Record(data) => { | ||
for field in data.fields.values() { | ||
if let Some(v) = &field.value { | ||
let value_with_ctr = RuntimeContract::apply_all( | ||
v.clone(), | ||
field.pending_contracts.iter().cloned(), | ||
v.pos, | ||
); | ||
eval_permissive(vm, errors, value_with_ctr); | ||
} | ||
} | ||
} | ||
_ => {} | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not set, but I wonder if that should move to VirtualMachine
instead. After all it's yet another mode of evaluation, and the code is quite similar to other VM's functions, while it stands out here compared to the rest (which is more about process intercommunication).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave that a try and it does seem nicer. My main concern was that this behavior is a bit LSP-specific.
|
||
fn handle_command(&mut self, msg: Command) -> Result<(), SupervisorError> { | ||
if let Command::UpdateFile { uri, text } = &msg { | ||
self.contents.insert(uri.clone(), text.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also remove the file from the banned ones on update? Or maybe a timeout is indeed a better way, or a mix of the two (only remove after timeout + update). But that can wait for another PR, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mainly worried about working on files that take a long time to evaluate. The editor might update the contents several times per second, and so if we keep on trying to evaluate and timing out then we're just pegging their CPU for nothing.
I agree that banning the file forever is probably too aggressive and I haven't thought too carefully about better policies. I think in the longer term, incremental evaluation would solve this problem: even if evaluation is slow, it's making progress building the incremental cache and so future evals will be fast.
fn run(&mut self) { | ||
loop { | ||
match self.run_one() { | ||
Ok(_) => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Ok
is unreachable? Might be worth a small comment (like an unwap
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially had a blocking question, but I realized midway that I misunderstood the code and removed it, so in fact it's good to go (of course modulo what you want to make out of the other comments - but nothing blocking).
Doesn't support termination, which is probably not good enough
This is a basic version of eval-in-the-background for the LSP. It has plenty of deficiencies, but I think it's a step in the right direction.
We spawn one subprocess in the background and keep its state up-to-date with the current files open in the editor. We evaluate files each time they are updated, monitoring the background process and killing it if it goes on too long. If a file times out, we blacklist it and don't try evaluating it again.
Among the many unfinished parts:
We're just runningeval_full
and reporting the first error.