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

excessive wakeups/cpu usage with mode=raw jobs with close_cb but not out_cb #10758

Closed
liskin opened this issue Jul 20, 2022 · 4 comments
Closed
Labels

Comments

@liskin
Copy link

liskin commented Jul 20, 2022

Steps to reproduce

  1. :let job = job_start('sh -c "echo x; cat"', {"mode": "raw", "close_cb": {->0}})
  2. strace -f -p <pid-of-vim>

Expected behaviour

I expected to see the same output as I see after :let job = job_start('sh -c "echo x; cat"', {"mode": "raw", "close_cb": {->0}, "out_cb": {->0}}), that is:

pselect6(11, [0 6 8 10], [], [0], NULL, NULL

but what I got instead was a lot of

pselect6(14, [0 6 8 10], [], [0], {tv_sec=0, tv_nsec=10000000}, NULL) = 0 (Timeout)

I investigated this a bit in gdb and found it being caused by

vim/src/ui.c

Lines 370 to 371 in b74e046

if (channel_any_readahead())
wait_time = 10L;

What is the point of this polling? Is it useful for raw channels? I gitblamed it back to 8a8199e but can't say I understand the motivation.

(This is a real issue in https://github.com/dense-analysis/ale introduced recently by an addition of close_cb to a job without an err_cb, but it'll need to be fixed there anyway because a) needs a fix for older vim versions and b) accumulating stderr in some unbounded buffer somewhere is bad)

Version of Vim

9.0.0057

Environment

OS: Linux (Debian testing)
the rest is irrelevant I believe

Logs and stack traces

No response

@liskin liskin added the bug label Jul 20, 2022
liskin added a commit to liskin/ale that referenced this issue Jul 20, 2022
When 'close_cb' is set for job_start(), but out_cb or err_cb isn't, vim
buffers data instead of dropping it (in case someone wanted to read and
process it in close_cb), and additionally polls for new data every 10
milliseconds, causing excessive wakeups and CPU usage. Since we don't
read the data anywhere outside of out_cb/err_cb, any LSP that prints an
error to stderr triggers this and vim keeps spinning until :ALEStopAllLSPs.

Fix this by always setting both callbacks, thus dropping any data we're
not interested in.

See vim/vim#10758 for an upstream report of
the excessive polling. It's possible this is intentional, I dunno.

Fixes: b42153e ("Fix dense-analysis#4098 - Clear LSP data when servers crash")
@brammool
Copy link
Contributor

brammool commented Jul 25, 2022 via email

@liskin
Copy link
Author

liskin commented Jul 25, 2022

The idea here, if I remember correctly, is that if there is a partly received message, then we should check again soon to see if the rest of the message is now available.
Perhaps in your specific situation there is readahead but checking for more will never read anything?

The specific situation is mode=raw and no callback, so "there is readahead" just means there's pending input that hasn't been read and will not be read because there's no callback to read it, and also there's no known format/structure of the input that would suggest it's a partial input or not.

Still, my question was a bit more general: when there is a known format/structure (e.g. json), why does vim need to do frequent polling instead of just waiting for (p)select to tell it there's new input?

hsanson pushed a commit to dense-analysis/ale that referenced this issue Jul 26, 2022
When 'close_cb' is set for job_start(), but out_cb or err_cb isn't, vim
buffers data instead of dropping it (in case someone wanted to read and
process it in close_cb), and additionally polls for new data every 10
milliseconds, causing excessive wakeups and CPU usage. Since we don't
read the data anywhere outside of out_cb/err_cb, any LSP that prints an
error to stderr triggers this and vim keeps spinning until :ALEStopAllLSPs.

Fix this by always setting both callbacks, thus dropping any data we're
not interested in.

See vim/vim#10758 for an upstream report of
the excessive polling. It's possible this is intentional, I dunno.

Fixes: b42153e ("Fix #4098 - Clear LSP data when servers crash")
@brammool
Copy link
Contributor

brammool commented Jul 26, 2022 via email

@liskin
Copy link
Author

liskin commented Aug 30, 2022

You could add a callback that discards the text, then there won't be this unused typahead.

Sure, that's what I ended up doing, it just seemed weird having to do this, with the docs not saying something like "the input must be read or else!" :-)
But then yeah, it's fairly obvious there'd be a buffer somewhere filling up, and the excessive wakeups is just a minor issue in many cases.

@liskin liskin closed this as completed Aug 30, 2022
mnikulin pushed a commit to mnikulin/ale that referenced this issue Nov 12, 2023
…#4259)

When 'close_cb' is set for job_start(), but out_cb or err_cb isn't, vim
buffers data instead of dropping it (in case someone wanted to read and
process it in close_cb), and additionally polls for new data every 10
milliseconds, causing excessive wakeups and CPU usage. Since we don't
read the data anywhere outside of out_cb/err_cb, any LSP that prints an
error to stderr triggers this and vim keeps spinning until :ALEStopAllLSPs.

Fix this by always setting both callbacks, thus dropping any data we're
not interested in.

See vim/vim#10758 for an upstream report of
the excessive polling. It's possible this is intentional, I dunno.

Fixes: b42153e ("Fix dense-analysis#4098 - Clear LSP data when servers crash")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants