style(bash-v2): out is not an array variable, do not refer to it as such #1681

scop · 2022-04-27T20:33:59Z

Even though this to my surprise works, it doesn't accomplish anything but some confusion. Remove it.

scop · 2022-04-27T20:35:07Z

Just something trivial to get out of the way for other likely improvements related to #1680.

marckhouzam

Hmmm... I'm not sure why I used out as an array variable. It might have started as an array in the early iterations of the script and then I didn't notice it wasn't one anymore.

But thanks @scop for cleaning that up.
I have tested with the cobra-completion-testing repo and with helm and it looks great.

style(bash-v2): out is not an array variable, do not refer to it as such Even though this to my surprise works, it doesn't accomplish anything but some confusion. Remove it. Merge spf13/cobra#1681 --- perf(bash-v2): use backslash escape string expansion for tab Using a command substitution, i.e. a subshell, with `printf` is expensive for this purpose. For example `__*_format_comp_descriptions` is run once for each completion candidate; the expense adds up and shows when there are a lot of them. This shaves off roughly 25% of the times I receive for my test case in #1680 (~2 seconds before, ~1.5 after). Merge spf13/cobra#1682 --- perf(bash-v2): standard completion optimizations Refactor to remove two loops over the entire list of candidates. Format descriptions only for completions that are actually going to be displayed, instead of for all candidates. Format descriptions inline in completions array, removing need for a command substitution/subshell and a printf escape per displayed completion. These changes shave off a second or so from my case at hand in #1680, it was roughly ~1.7s before, is now ~0.7s after. The diff is largish, but the bulk of it is in `__%[1]s_format_comp_descriptions` due to indentation changes, there aren't that many lines actually changed in it either. `git show -w` helps with the review. Caveat: I have not actually tested generating completions with this code. I worked on the improvements by tweaking an existing emitted completion shell file, then manually ported the changes to the template in Go code here. Hopefully I didn't introduce bugs while doing that. (The code compiles, and test suite fails just like it did before this change, no regressions noticed.) Merge spf13/cobra#1683 --- perf(bash-v2): short-circuit descriptionless candidate lists If the list of candidates has no descriptions, short circuit all the description processing logic, basically just do a `compgen -W` for the whole list and be done with it. We could conceivably do some optimizations like this and more when generating the completions with `--no-descriptions` in Go code, by omitting some parts we know won't be needed, or doing some things differently. But doing it this way in bash, the improvements are available also to completions generated with descriptions enabled when they are invoked for completion cases that produce no descriptions. The result after this for descriptionless entries seems fast enough so it seems there's no immediate need to look into doing that. This alone pretty much eliminates all the slowness related to my specific use case in #1680, bringing it down to ~0.07s (yes, there _is_ a zero on the right of the period), so for the time being I'm not inclined to look into further optimizations at least for the code path where there are no descriptions. Related to #1683, but I suggest merging both as they affect different scenarios. Same caveat as in #1683 about testing and the way the change was made applies here. Merge spf13/cobra#1686 --- perf(bash-v2): speed up filtering entries with descriptions Use simple prefix match instead of single word `compgen -W` command substitution for each candidate match. It's a bit late, but I can't think of a case where this replacement wouldn't be ok this late at night. Performancewise this improves the cases with descriptions quite a bit, with current marckhouzam/cobra-completion-testing#15 on my box, the v2 numbers look like the following (I've stripped the v1 numbers from each list below). Before (current master, #1686 not merged yet): ``` Processing 1000 completions took 0.492 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.600 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.455 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.578 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.424 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.570 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.502 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.585 seconds which is less than the 2.0 seconds limit ``` After (also without #1686): ``` Processing 1000 completions took 0.070 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.165 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.072 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.181 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.089 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.254 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.058 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.122 seconds which is less than the 2.0 seconds limit ``` For curiosity, even though this improves the descriptionless case quite a bit too, #1686 is still relevant, with it applied on top of this one: ``` Processing 1000 completions took 0.036 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.165 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.047 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.183 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.051 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.264 seconds which is less than the 2.0 seconds limit Processing 1000 completions took 0.036 seconds which is less than the 1.0 seconds limit Processing 1000 completions took 0.122 seconds which is less than the 2.0 seconds limit ``` Merge spf13/cobra#1689 --- fix(bash-v2): skip empty completions when filtering descriptions `read` gives a last null value following a trailing newline. Regression from fb8031162c2ffab270774f13c6904bb04cbba5a7. We could drop the `\n` from the feeding `printf` to accomplish the same end result, but I'm planning to submit some related cleanups a bit later that wouldn't play well with that approach, so I went with explicit filtering here. Merge spf13/cobra#1691 --- perf(bash-v2): speed up filtering menu-complete descriptions Similarly as fb8031162c2ffab270774f13c6904bb04cbba5a7 (+ the empty entry fix in spf13/cobra#1691) did for the "regular", non-menu completion cases. I have no numbers on this, but I'm assuming the speedups seen elsewhere are here too, and didn't notice anything breaking in casual manual tests. Could be useful to add some perf numbers for the menu-complete code path too to [marckhouzam/cobra-completion-testing](https://github.com/marckhouzam/cobra-completion-testing/) (maybe as well as other tests, haven't checked if there are any). Merge spf13/cobra#1692 --- perf(bash-v2): read directly to COMPREPLY on descriptionless short circuit Not that it'd really matter that much performancewise given the level we are at for this case, but this change makes the short circuit roughly twice as fast on my box as it was for the 1000 rounds done in marckhouzam/cobra-completion-testing. Perhaps more importantly, this makes the code arguably slightly cleaner. Before: ``` ... <= TIMING => no descriptions: 1000 completions took 0.039 seconds < 0.2 seconds limit ... <= TIMING => no descriptions: 1000 completions took 0.046 seconds < 0.2 seconds limit ... ``` After: ``` ... <= TIMING => no descriptions: 1000 completions took 0.022 seconds < 0.2 seconds limit ... <= TIMING => no descriptions: 1000 completions took 0.024 seconds < 0.2 seconds limit ... ``` Merge spf13/cobra#1700

style(bash-v2): out is not an array variable, do not refer to it as such

82e85b5

Even though this to my surprise works, it doesn't accomplish anything but some confusion. Remove it.

github-actions bot added the size/S Denotes a PR that chanes 10-24 lines label Apr 27, 2022

marckhouzam approved these changes Apr 28, 2022

View reviewed changes

marckhouzam merged commit 4f0facb into spf13:master Apr 28, 2022

marckhouzam added this to the 1.5.0 milestone Apr 28, 2022

marckhouzam added the area/shell-completion All shell completions label Apr 28, 2022

scop deleted the style/bash-v2-out branch April 28, 2022 05:23

umarcor mentioned this pull request May 1, 2022

Spring 2022 release 🌼 #1626

Closed

sascha1337 mentioned this pull request Aug 6, 2022

Bump github.com/spf13/cobra from 1.4.0 to 1.5.0 CosmosContracts/juno#252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style(bash-v2): out is not an array variable, do not refer to it as such #1681

style(bash-v2): out is not an array variable, do not refer to it as such #1681

scop commented Apr 27, 2022

scop commented Apr 27, 2022

marckhouzam left a comment

style(bash-v2): out is not an array variable, do not refer to it as such #1681

style(bash-v2): out is not an array variable, do not refer to it as such #1681

Conversation

scop commented Apr 27, 2022

scop commented Apr 27, 2022

marckhouzam left a comment

Choose a reason for hiding this comment