Add test of performance for bash completion #15
Merged
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
marckhouzam
force-pushed
the
feat/testSpeed
branch
from
91e09c3
to
231c177
Compare
|
Very nice 👍. I'd suggest adding something providing benchmarkability also for completions without descriptions, because the code paths and optimization opportunities may differ. For example a command line boolean flag to Perhaps also a "mixed" mode test where some completion candidates have descriptions and some don't. This isn't terribly interesting in benchmark sense, but would be good to verify that the case works, too. In that sense also perhaps out of scope for this PR. |
marckhouzam
force-pushed
the
feat/testSpeed
branch
from
231c177
to
d74c92b
Compare
Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
marckhouzam
force-pushed
the
feat/testSpeed
branch
from
d74c92b
to
4983115
Compare
As you later noticed the test suite runs test without descriptions and with descriptions 👍
I agree, out of scope for this PR, but the idea is not without merit. |
|
I had left an 'exit' in the PR which I had added just to speed up my tests. That's what happens when there is no code review 😄 |
hoshsadiq
pushed a commit
to zulucmd/zulu
that referenced
this pull request
Dec 31, 2022
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
hoshsadiq
added a commit
to zulucmd/zulu-completion-testing
that referenced
this pull request
Jan 2, 2023
Performance tests were added (marckhouzam#15) Add dirs that caused tests to fail (marckhouzam#18) Fixed all other tests for zulu Co-authored-by: Marc Khouzam <marc.khouzam@gmail.com> Co-authored-by: =?UTF-8?q?Ville=20Skytt=C3=A4?= <ville.skytta@upcloud.com>
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.
The bash completion v2 script is currently very slow. These tests will help prevent a regression once we've improved the v2 script.
See spf13/cobra#1680
I'll wait until we have improved the script before merging.