Skip to content

perf: Loop over the bash variable directly instead of starting subprocesses.#2333

Merged
marckhouzam merged 1 commit intospf13:mainfrom
JeffFaer:jfaer/opt
Dec 6, 2025
Merged

perf: Loop over the bash variable directly instead of starting subprocesses.#2333
marckhouzam merged 1 commit intospf13:mainfrom
JeffFaer:jfaer/opt

Conversation

@JeffFaer
Copy link
Copy Markdown
Contributor

@JeffFaer JeffFaer commented Nov 25, 2025

Today, the loop is printing the completions one value per line and then reading one line to a variable. We can just skip the whole IO rigamarole by looping over the completions directly.

I had added this optimization to #2126 but it doesn't look like it got picked up by #1743.

…cesses.

Today, the loop is printing the completions one value per line and then
reading one line to a variable. We can just skip the whole IO rigamarole
by looping over the completions directly.
@marckhouzam
Copy link
Copy Markdown
Collaborator

Will this cause problems if some completions have a space in them?

@JeffFaer
Copy link
Copy Markdown
Contributor Author

It shouldn't. The quotes around the array variable should prevent word splitting like that.

$ arr=( "one" "two" "three four" "five")
$ for n in "${arr[@]}"; do
  echo "$n"
done
$ for n in ${arr[@]}; do
  echo "$n"
done

@marckhouzam
Copy link
Copy Markdown
Collaborator

marckhouzam commented Nov 25, 2025

Ok. Give me a bit of time to revive the completion tests that I haven't looked at in a while and run them.
Ref: https://github.com/marckhouzam/cobra-completion-testing

Copy link
Copy Markdown
Contributor

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick spot check with podman and it seems to work correctly even if the values have spaces in them.

@JeffFaer
Copy link
Copy Markdown
Contributor Author

JeffFaer commented Dec 2, 2025

Anecdotally, this appears to shave off something like 10ms:

#!/usr/bin/env bash

arr=()
for ((i=0; i<1000; i++)); do
    arr+=($i)
done

trials=1

loop_arr_1() {
    sum=0
    for ((j=0; j<trials; j++)); do
        for i in "${arr[@]}"; do
            ((sum+=i)) || true
        done
    done
}

loop_arr_2() {
    sum=0
    for ((j=0; j<trials; j++)); do
        while IFS='' read -r i; do
            ((sum+=i)) || true
        done < <(printf '%s\n' "${arr[@]}")
    done
}

time loop_arr_1
time loop_arr_2
real	0m0.003s
user	0m0.003s
sys	0m0.000s

real	0m0.016s
user	0m0.013s
sys	0m0.004s

Copy link
Copy Markdown
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regressions tests pass.
LGTM

@marckhouzam marckhouzam merged commit 10d4b48 into spf13:main Dec 6, 2025
21 checks passed
@marckhouzam marckhouzam added this to the 1.11.0 milestone Dec 6, 2025
bejaratommy added a commit to bejaratommy/cobra that referenced this pull request Apr 11, 2026
…cesses. (spf13#2333)

Today, the loop is printing the completions one value per line and then
reading one line to a variable. We can just skip the whole IO rigamarole
by looping over the completions directly.
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.

3 participants