-
Notifications
You must be signed in to change notification settings - Fork 719
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
rc tools menu: work around ARG_MAX limit #5060
Open
krobelus
wants to merge
1
commit into
mawww:master
Choose a base branch
from
krobelus:fix-menu-performance
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
krobelus
added a commit
to kakoune-lsp/kakoune-lsp
that referenced
this pull request
Jan 10, 2024
Kakoune master learned a menu command with fuzzy completion. This obsoletes "lsp-menu". Let's copy it here until until we can expect it to be available. This fixes an issue where a large number of args makes us hit ARG_MAX, see mawww/kakoune#5060. Drops support for Kakoune versions where "prompt" does not have the "-menu" switch. This bumps the Kakoune version requirement to 2022.10.31 which is in Debian stable.
krobelus
force-pushed
the
fix-menu-performance
branch
from
January 19, 2024 04:04
ac84184
to
70f110a
Compare
when "menu" arguments exceed ~200k bytes, I get "execve failed: Argument list too long" (even though ARG_MAX is 2MB). This is because the shell process is passed the arguments to menu. Reproduce with evaluate-commands %exp{ menu asdf %sh{dd 2>/dev/null if=/dev/zero bs=1000 count=200 | sed s/././g} } I hit this with rust-analyzer which can send ~70 code actions to select from, each with a lengthy JSON object, so the :menu invocation can sometimes reach the effective limit. It can also become slow (0.5 seconds), maybe because we fork multiple times per argument. Fix this by passing arguments through $kak_response_fifo. The on-accept and on-change callbacks have the same problem (sh -c string too long), so move them to temporary files. This means we can get rid of some escaping. Note that there is currently no dedicated way to stop Kakoune from passing "$@" to the shell process, so define an extra command that doesn't take args. Since we serialize arguments into a single string (using "echo -quoting"), we need to deserialize them before doing anything else. We currently don't have an off-the-shelf solution for this. Perhaps this is an argument for "echo -quoting json" (which has been suggested before I believe).
krobelus
added a commit
to krobelus/kakoune
that referenced
this pull request
Aug 26, 2024
The current implementation of menu has scaling problems (it can get slow or fail due to ARG_MAX). Additionally, menus lack some of the flexibility and orthogonality of buffers. I can probably work around the scaling problems by limiting the number of menu entries. Still, the buffer approach seems worth exploring. Make menu use a scratch buffer instead of prompt. This drops support for -select-cmds and -on-abort, which are no longer necessary because the user can "preview" menu entries by using <ret> and ga in normal mode. This includes a number of hacks that we might want to get rid of: 0. It moves the implementation of -auto-single to a separate command. Otherwise it would be difficult to avoid ARG_MAX, though that's probably not a blocker. 1. When no completion was selected explicitly, insert-mode <ret> first injects a <c-n>. This is to support existing menu usage scenarios. There is no "-menu" switch (yet?) to mark insert-mode completions as authoritative. 2. The command is currently included in the buffer text (and in completion). This keeps things simple but it can be ugly for long commands. We might want to hide the command in a line-specs option. 3. The command is run in the context of the buffer that created the menu. This works well for use cases that have started relying on this (like lsp-code-actions) though we could try to change this. In future, we can try to also use scratch buffers for composing prompt commands, like "git edit ". Closes mawww#5060
krobelus
added a commit
to krobelus/kakoune
that referenced
this pull request
Oct 21, 2024
The current implementation of menu has scaling problems (it can get slow or fail due to ARG_MAX). Additionally, menus lack some of the flexibility and orthogonality of buffers. I can probably work around the scaling problems by limiting the number of menu entries. Still, the buffer approach seems worth exploring. Make menu use a scratch buffer instead of prompt. This drops support for -select-cmds and -on-abort, which are no longer necessary because the user can "preview" menu entries by using <ret> and ga in normal mode. This includes a number of hacks that we might want to get rid of: 0. It moves the implementation of -auto-single to a separate command. Otherwise it would be difficult to avoid ARG_MAX, though that's probably not a blocker. 1. When no completion was selected explicitly, insert-mode <ret> first injects a <c-n>. This is to support existing menu usage scenarios. There is no "-menu" switch (yet?) to mark insert-mode completions as authoritative. 2. The command is currently included in the buffer text (and in completion). This keeps things simple but it can be ugly for long commands. We might want to hide the command in a line-specs option. 3. The command is run in the context of the buffer that created the menu. This works well for use cases that have started relying on this (like lsp-code-actions) though we could try to change this. In future, we can try to also use scratch buffers for composing prompt commands, like "git edit ". Closes mawww#5060
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
when "menu" arguments exceed ~200k bytes, I get "execve failed:
Argument list too long" (even though ARG_MAX is 2MB).
Reproduce with
I hit this with rust-analyzer which can send ~70 code actions to
select from, each with a lengthy JSON object, so the :menu invocation
can sometimes reach the effective limit. It can also become slow
(0.5 seconds), maybe because we fork multiple times per argument.
Fix this by passing arguments through $kak_response_fifo.
The on-accept and on-change callbacks have the same problem (sh -c
string too long), so move them to temporary files. This means we
can get rid of some escaping.
Note that there is currently no dedicated way to stop Kakoune from
passing "$@" to the shell process, so define an extra command that
doesn't take args.
Since we serialize arguments into a single string (using "echo
-quoting"), we need to deserialize them before doing anything else.
We currently don't have an off-the-shelf solution for this. Perhaps
this is an argument for "echo -quoting json" (which has been suggested
before I believe).