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(lsp): enable default debounce of 150 ms #16908

Merged
merged 1 commit into from
Jan 5, 2022
Merged

feat(lsp): enable default debounce of 150 ms #16908

merged 1 commit into from
Jan 5, 2022

Conversation

mjlbach
Copy link
Contributor

@mjlbach mjlbach commented Jan 4, 2022

I think we should enable debounce by default. It's well-tested now and this should avoid issues with slower servers. It can still be disabled by setting debounce_text_changes to 0 ms. Ofc users can replicate this in 0.6.1 by setting flags/debounce_text_changes in lspconfig/start_client.

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 4, 2022

Need to fix tests.

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 5, 2022

@sam-mccall do you have any thoughts/preference on a sane default for our debounce (as our resident clangd expert :))? I picked 150 somewhat arbitrarily. CoC.nvim/vscode use 300 ms IIRC.

@sam-mccall
Copy link
Contributor

@mjlbach Sure thing, I spent a while thinking about this though it was a couple of years ago. Thanks for caring about this stuff, I think it makes a big difference.

TL;DR: 200ms or so seems reasonable to me, 150 maybe a little low vs typing speed. Depends a bit what you want to achieve with debounce. This a tradeoff about "feeling" so there's no perfect number.

Clangd has an internal debounce on change events that's adaptive to rebuild latency, so it might be nice to disable client-side debounce for clangd by default unless it resolves client-side performance issues.
(On the other hand a consistent experience across language servers is reasonable to want too)


AFAIK the main constraints/reasons for debounce are:
A. avoid distracting transient diagnostics while the user is typing
B. avoid bad diagnostics that hang around after finishing typing
C. minimize latency to get "good" diagnostics and other operations after finishing typing
D. client side reasons e.g. performance

For (A) the debounce should exceed the time between keystrokes. 300ms will be enough for most users, 150ms is probably only enough for fast typists.
For (B), if diagnostic cycle time is large, then debounce must exceed time between keystrokes.
For (C), should exceed time between keystrokes, but must not exceed diagnostic cycle time
I don't know much about (D)

My feeling was that priorities were B>C>A, so we set debounce = clamp{50, diagnostic cycle time, 500} inside clangd, and it feels OK.

If you think that A is more important, a high debounce like 500ms might be better.
My experience is this is a question of taste (some people complain, others want lower latency.) In a vim context update_in_insert may be a better fix. (Or not sending updates while in insert mode).

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 5, 2022

Thanks for the thoughtful reply!

TL;DR: 200ms or so seems reasonable to me, 150 maybe a little low vs typing speed.

I can bump 150 up to 200 (or alternatively handle this only in lspconfig).

Depends a bit what you want to achieve with debounce. This a tradeoff about "feeling" so there's no perfect number.

My main priority is "responsiveness". It's a bit hard to come up with a universal number that works well for everyone, as I believe hardware will also play a strong role here.

Clangd has an internal debounce on change events that's adaptive to rebuild latency, so it might be nice to disable client-side debounce for clangd by default unless it resolves client-side performance issues.

We could still do this per server in lspconfig. An alternative would be to not handle setting the default debounce in core, although then plugin authors will need to think about this.

AFAIK the main constraints/reasons for debounce are:
A. avoid distracting transient diagnostics while the user is typing
B. avoid bad diagnostics that hang around after finishing typing
C. minimize latency to get "good" diagnostics and other operations after finishing typing
D. client side reasons e.g. performance

Personally only C. matters to me, I think we should also optimize for request time, that is, the debounce should be as such to minimize the time the servers spends processing the last notification(s) to optimize the time required on the first subsequent request. The most concrete example of this would be servicing completion requests.

@sam-mccall
Copy link
Contributor

Depends a bit what you want to achieve with debounce. This a tradeoff about "feeling" so there's no perfect number.

My main priority is "responsiveness". It's a bit hard to come up with a universal number that works well for everyone, as I believe hardware will also play a strong role here.

👍 Different language servers may have different performance patterns too. (As well as "slowness", what's serial vs parallel matters a lot here).

Clangd has an internal debounce on change events that's adaptive to rebuild latency, so it might be nice to disable client-side debounce for clangd by default unless it resolves client-side performance issues.

We could still do this per server in lspconfig. An alternative would be to not handle setting the default debounce in core, although then plugin authors will need to think about this.

Lspconfig sounds sensible from a layering perspective, and if ~everyone uses lspconfig, it probably doesn't make a practical difference. Apart from the ability to give better per-server defaults :-)

Personally only C. matters to me, I think we should also optimize for request time, that is, the debounce should be as such to minimize the time the servers spends processing the last notification(s) to optimize the time required on the first subsequent request. The most concrete example of this would be servicing completion requests.

Yeah, this depends on what's parallel vs serial in the server (and also whether constrained by hardware).
(clangd actually has a weird model where all operations on a file are serialized except completion, for sad reasons)

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 5, 2022

Lspconfig sounds sensible from a layering perspective, and if ~everyone uses lspconfig, it probably doesn't make a practical difference. Apart from the ability to give better per-server defaults :-)

(sorry this will be off-topic) Eventually I don't want everyone to use lspconfig, I'd rather defer to dedicated language plugins. I've been planning to write a "developing a dedidcated language plugin for neovim" guide for awhile which could cover setting a debounce per server.

I think one advantage of default debounce in core is there are many non-singular operations (like Join, which triggers on_lines 3 times) that are effectively instant. Adding a small debounce in these cases (I believe) is useful for servers, although as you say that depends on the implementation of the server (if it handles internal debounce, etc.). I may hold on this for now and add the per-server debounce in lspconfig (disabled for clangd as you say).

Anyways, now I am torn :)

@sam-mccall
Copy link
Contributor

sam-mccall commented Jan 5, 2022

(sorry this will be off-topic) Eventually I don't want everyone to use lspconfig, I'd rather defer to dedicated language plugins. I've been planning to write a "developing a dedidcated language plugin for neovim" guide for awhile which could cover setting a debounce per server.

Maybe offtopic but very interesting! This is the VSCode/coc model of course and a little bit of a maintenance headache for LS authors, but makes the editor a much more attractive target for LSP extensions/quirks.

It would help if these extensions can defer to some default facility for things like how to handle "standard" setup{} options only customizing if needed. IDK if that layer would be core or some other library, but I guess that'd be where defaults like debounce belong.

I think one advantage of default debounce in core is there are many non-singular operations (like Join, which triggers on_lines 3 times) that are effectively instant. Adding a small debounce in these cases (I believe) is useful for servers, although as you say that depends on the implementation of the server (if it handles internal debounce, etc.).

Well, it won't hurt either. I think a client-side debounce of 10ms to cover compound edits is a great idea, maybe even as a forced unconfigurable minimum.
(Now that I think about it, I remember a client that made x11-pasting really slow just due to thrashing the client with no debounce).

Anyways, now I am torn :)

Sorry :-)
FWIW, now I understanding your plan to get people off lspconfig, putting this in core seems pretty reasonable to me. lspconfig could still override...

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 5, 2022

Maybe offtopic but very interesting! This is the VSCode/coc model of course and a little bit of a maintenance headache for LS authors, but makes the editor a much more attractive target for LSP extensions/quirks.

Yes, I'm planning on moving the "core" of lspconfig (generic project detection, automatically attaching all buffers to a language server watching a directory) soon-ish, I started moving the path utilities over last week (with some requisite bikeshedding). I want to move away from a "server centric" project like lspconfig is currently to a "language level project" like configuration. The idea is to allow users to more precisely delegate different functionality to different language servers per project by exposing client ids directly to the "project" (and allow for more broad configuration of project level settings which people are abusing on_attach for currently).

Lspconfig will be renamed "server-configurations" and serve as a source of sane defaults for clients, but the actual server launching/attaching/project management utilties will be provided by core (and can be treated as a library for a future clangd.nvim that exposes all of the nice off-protocol clangd requests :))

@mfussenegger
Copy link
Member

mfussenegger commented Jan 5, 2022

I'd stick with the 150ms

Not sure if clangd is the best example here to talk about what kind of values makes most sense - as clangd is generally not problematic without any debounce at all. It is other servers which cannot keep up with a high frequency of didChange notifications that cause problems ("Why is LSP so slow") and motivated this.

@mjlbach mjlbach merged commit 55a59e5 into neovim:master Jan 5, 2022
@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 5, 2022

Yeah I merged it. We can always adjust the default value later. I can set a lower default for lspconfig for clangd (@sam-mccall if you want to chat neovim lsp stuff you should hop on matrix because I would value your input in the future)

@sam-mccall
Copy link
Contributor

Thanks, will do! (though I have trouble keeping up with too many info sources)

I'd forgotten about the serverside latency problems if they're not folding notifications together (or that itself is too slow). 150ms makes total sense for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants