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

rc tools menu: work around ARG_MAX limit #5060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krobelus
Copy link
Contributor

@krobelus krobelus commented Dec 16, 2023

when "menu" arguments exceed ~200k bytes, I get "execve failed:
Argument list too long" (even though ARG_MAX is 2MB).

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 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 krobelus force-pushed the fix-menu-performance branch from ac84184 to 70f110a Compare January 19, 2024 04:04
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant