Skip to content
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

Don't leak memory in background eval #1869

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Don't leak memory in background eval #1869

merged 5 commits into from
Mar 26, 2024

Conversation

jneem
Copy link
Member

@jneem jneem commented Mar 25, 2024

This fixes a memory leak in background evaluation, by running each background eval job in a separate process. This might be a little extreme: there's a middle ground where we run several eval jobs in a single process before killing it to reclaim the memory. This extreme approach has the advantage of being simpler and not having an additional knob to tune.

@jneem jneem requested review from yannham and vkleen March 25, 2024 22:26
@github-actions github-actions bot temporarily deployed to pull request March 25, 2024 22:28 Inactive
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

If I understand correctly, does this mean we don't dedup anymore, and just blindly spawn a new process as soon as we have a notification? Is that at risk of spawning many many processes during live editing?


contents: HashMap<Url, String>,
deps: HashMap<Url, Vec<Url>>,
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to running the process in the background? Why do we need to keep deps now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we spin up a new process for each job, we need to transfer over all the file contents (because the LSP's in-memory contents might be different to what's on disk). The dependency tracking allows us to send over only the file contents that the current eval depends on.

@jneem
Copy link
Member Author

jneem commented Mar 26, 2024

No, it should still only eval a single file at a time, although the mechanism is somewhat different: the main process has a single background thread in charge of the eval processes. The background thread spawns an eval process and then waits for it to finish. While it's waiting, new requests from the main thread can pile up in its channel. When it's done waiting, it drains all those requests and dedups them (in SupervisorState::handle_command).

I did just realize that we aren't killing the child when it times out, though, so let me fix that...

Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

I see, thanks for the precision. The approach looks good to me: while there's space for fine-tuning, even if we waste a bit of resources spanning a lot of processes, as long as it doesn't happen in the main LSP thread, it's reasonable.

@github-actions github-actions bot temporarily deployed to pull request March 26, 2024 18:18 Inactive
@jneem jneem added this pull request to the merge queue Mar 26, 2024
Merged via the queue into master with commit 1caa77e Mar 26, 2024
5 checks passed
@jneem jneem deleted the background-eval-memory branch March 26, 2024 20:21
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.

2 participants