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

feat(LspconfigServerLifeCycle): stop/start server upon demand #2609

Closed
wants to merge 2 commits into from

Conversation

hinell
Copy link

@hinell hinell commented May 12, 2023

Start/stop servers when nvim session looses focus and restart them on demand.
This features is motivated by the fact that some servers may gain serious
memory footprint and may incur performance issues.

UPDATE: Implementd by a separate plugin: hinell/lsp-timeout.nvim

@hinell hinell changed the title feat(LspconfigServerLifeCycle): Start/stop servers when gained or los… feat(LspconfigServerLifeCycle): Start/stop servers when gained or lost focus May 12, 2023
@hinell hinell changed the title feat(LspconfigServerLifeCycle): Start/stop servers when gained or lost focus feat(LspconfigServerLifeCycle): stop/start server upon demand May 12, 2023
@lithammer
Copy link
Collaborator

lithammer commented May 12, 2023

This seems like a very drastic solution. Also, say I work on project (workspace) and open file A to quickly edit something and then move on to file B. Now after 5 minutes the server would be stopped in middle of editing B? And what's worse is that it wouldn't start again until I move back to A (or some new file C) or start it manually.

I often work in projects where it can easily take 30s before the language server is ready. And having that happen because I stopped to edit the same buffer for 5 min would be frustrating.

I think a 30-60 min timeout would be more reasonable. And it would be have to be tracked per language server instance.

Or maybe I'm misunderstanding how it works?

@sigmaSd
Copy link
Contributor

sigmaSd commented May 12, 2023

1- What do other editors do (vscode in particular) ?
2- Do all lsp servers handle this stopping/starting gracefully ?

In my opinion this should not be a default behavior
This should be opt in for users who cares about this, or just even mentioned in the docs

@hinell
Copy link
Author

hinell commented May 12, 2023

@lithammer This won't stop servers local to some specific language or buffer if you switch between them. I've tweaked to stop server only if nvim window looses focus.
@sigmaSd I wanted to make it optional, but there is no lspconfig.setup() function in this project.

@lithammer
Copy link
Collaborator

lithammer commented May 12, 2023

@lithammer This won't stop servers local to some specific language or buffer if you switch between them. I've tweaked to stop server only if nvim window looses focus.

Ah, it was BufLeave and BufEnter when I first looked. I still think the 5 min timeout is too low.

@hinell
Copy link
Author

hinell commented May 12, 2023

@lithammer Fixed it. My mistake. If we hook into buffers lifecycle it will constantly create/destroy timer. This may create problems if someone switches between buffers often.

…sing focus

Start/stop servers when nvim session looses focus and restart them on demand.
This features is motivated by the fact that some servers may gain serious
memory footprint and may incur performance issues.
@lithammer
Copy link
Collaborator

Still, 5 min will go by fast. Just writing this comment would cause a language server to be stopped.

@hinell
Copy link
Author

hinell commented May 12, 2023

@lithammer It won't stop servers if you don't move away from nvim window. But if you switch to some another window it will (e.g. browser). Should I set timeout to 30 minutes?

I think we need a sane setup(config) function in this project so it can be configurable. 🙄 I can add global config field but is it going to be acceptable?

@gegoune
Copy link
Contributor

gegoune commented May 12, 2023

That definitely should not be default behaviour. Ever. No matter the timeout. Some people might be reading docs while coding. ;)

@hinell hinell force-pushed the lsplifecycle branch 2 times, most recently from 6ab1cce to 6520eb9 Compare May 12, 2023 17:08
@hinell
Copy link
Author

hinell commented May 12, 2023

@gegoune Checkout. The feature is now disabled by default and can only be enabled by setting a global vim.g.lspconfigServerLifeCycle = {} option.

Default timeout is now set to 30 minutes. Compare two commits.

@glepnir
Copy link
Member

glepnir commented May 13, 2023

A shutdown restart caused some language server cache calculations to be lost. What performance problem? Is this something the client should deal with. Obviously not. This is an implementation issue of the server. nothing to do on there.

@hinell
Copy link
Author

hinell commented May 13, 2023

@glepnir Some servers may easily take up gigabytes of memory. Keeping cache in memory is problem of servers, not LSP clients. They should not rely on RAM in this sense. I have several editors opened at the same time and can tell you they take up 150MB on average at least.

@glepnir
Copy link
Member

glepnir commented May 13, 2023

what i mean client is vim/neovim emacs vscode . This is not something the neovim client should deal with. It is normal for the server to consume memory. No matter what editor you use, this will happen as long as you use lsp. A short reboot doesn't fix the problem. eg rust_analyzer uses the arena memory allocator. It does not free memory. vscode vim-lsp emacs lsp-mode helix I don't see this either. I can't think of any reason to merge this pr. Sorry. Just my personal opinion. You can ask other people for their opinion.

@hinell
Copy link
Author

hinell commented May 13, 2023

Cache is a not problem. Optimized servers keep theer cache on HDD instead of memory to make it shareable. This PR is intended for optimization as it requires lspconfig commands.

@hinell
Copy link
Author

hinell commented Sep 18, 2023

Please, follow this repo for progress on standalone plugin:

@hinell hinell closed this Sep 18, 2023
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.

5 participants