bash completion v2 is much slower than v1

[scop]

I have a work-in-progress completion that returns 788 items. With cobra's bash completion v1, the results are pretty much instant, but with v2 they take seconds.

Results for v1 vs v2 completions generated by the same executable, the only difference being that the v1 completion is output "manually", and the v2 using cobra's default completion emitter, both built with cobra v1.4.0, both shell files manually modified so that the contents of __start_sshi is wrapped in time { ... } for benchmarking, v1:

$ sshi ssh <tab>
real	0m0,127s
user	0m0,089s
sys	0m0,040s

Display all 788 possibilities? (y or n)^C

v2:

$ sshi ssh <tab>
real	0m2,285s
user	0m1,669s
sys	0m0,713s

Display all 788 possibilities? (y or n)^C

For v2, that 2.2 seconds is pretty much the best case. Times like 3.3 and 3.8 seconds are not that rare. v1 is about always < 0.2 seconds.

[scop]

The project mentioned is https://github.com/aakso/ssh-inscribe, PR implementing (implicit) switch from the old completions to the V2 ones emitted by the default is aakso/ssh-inscribe#21, completion giving those ~800 completions in a test setup of mine is aakso/ssh-inscribe#20 (but the slowness isn't creating the list, but processing it in bash).

Generating the V2 completions with --no-descriptions shaves a few tenths of a second of the completion runtime, but it's still ~2 seconds.

[marckhouzam]

The v1 script does have an avantage over the v2 one in the fact that the static completions are hard-coded into the script. With v2, as you mention, your binary is explicitly called to get the completions. Can you run

time sshi __complete ssh ''

to see how long it takes your sshi binary to compute the completions? If it takes multiple seconds we'll have to look at why.

[scop]

$ time sshi __complete ssh '' | wc -l
Completion ended with directive: ShellCompDirectiveNoFileComp
810

real	0m0,048s
user	0m0,030s
sys	0m0,029s

I failed to bring up more prominently that I already checked that the completion slowness/most of the time is in bash. Neither v1 nor v2 in this case embed the completions in question, they're parsed dynamically from ssh known_hosts files on the fly.

Looking at only __sshi_handle_standard_completion_case, it seems on surface that there are possibilities to make it go faster -- it takes multiple passes over the entire list (maybe possible to get rid of some passes altogether in general, or at least if not caring about descriptions), and invokes at least one subshell for every entry in two of the loops.

[marckhouzam]

@scop Thanks for the info! It does seem to point at inefficiencies in the script itself. This is good news as we can surely improve it. I will try to find time to investigate but if you want to try your hand at it, don't hesitate.

[scop]

Thanks for looking into the low hanging fruit PR's so quickly. I have a slightly more involved, promising looking one in draft stage locally, will try to get it cleaned up and sliced/diced into smaller easier reviewable commits later today.

[marckhouzam]

Thanks for looking into the low hanging fruit PR's so quickly. I have a slightly more involved, promising looking one in draft stage locally, will try to get it cleaned up and sliced/diced into smaller easier reviewable commits later today.

Thanks a lot for your efforts! Your bash expertise is of great value. I'm glad the script will be optimized before more and more consumers move from the v1 to the v2.

[scop]

Ok, with #1683 and #1686, as far as I'm concerned we can call this done. After them, for my particular test case, I'm not able to tell a definite difference between v1 and v2, and some of the improvements made surely have had a positive effect on other v2 cases as well (no numbers on them, though).

Here's hoping I haven't worked on any assumptions that wouldn't hold or added any regressions in those PR's that would eliminate their 🤞

[marckhouzam]

I've posted a PR that adds a rudimentary performance test to help us verify performance improvements and possible future regressions: marckhouzam/cobra-completion-testing#15

[scop]

Per the summary at #1689 (comment) (+ one more for menu-complete in #1692) I think we can call this done.