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

ssh: optimize detection #2774

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

ssh: optimize detection #2774

wants to merge 1 commit into from

Conversation

visigoth
Copy link

p10k's ssh detection is pretty thorough, but also pretty heavy, especially when the primary use case is local. on my m1 macbook, (( $+commands[who] )) && return costs 7 milliseconds.

this PR changes $+commands[who] to command -v who, which runs almost instantaneously in comparison.

the next biggest cost is in the regex match at the end of the function. it feels like ssh detection should be an optional thing. the best way i've found to bypass it is to set P9K_SSH and faking out _P9K_TTY, and forcing detection on SSH_CLIENT or SSH_CONNECTION alone, which i'm fine with.

@Syphdias
Copy link
Contributor

How did you benchmark that the new comparison is faster? And how big is commands for you?

@Syphdias
Copy link
Contributor

I had a hard time believing it and needed to give this a try myself. Note I am no Linux, but this shouldn't be that different on an M1

#!/usr/bin/env zsh
COUNT=${COUNT:-10000}
function old() {
    (( $+commands[who] )) || return
}
function new() {
    [[ -z "$(command -v who)" ]] && return
}

t0=$(date +%s)

for i in {0..$COUNT}; do
    old
done

t1=$(date +%s)

for i in {0..$COUNT}; do
    new
done

t2=$(date +%s)

for i in {0..$COUNT}; do
    (( $+commands[who] ))
done

t3=$(date +%s)

for i in {0..$COUNT}; do
    [[ -z "$(command -v who)" ]]
done

t4=$(date +%s)

{
    echo "command total average"
    echo "+commandsF: $((t1-t0))s $(bc -l <<< "scale=5; 1000*($t1-$t0)/$COUNT")ms"
    echo "command-vF: $((t2-t1))s $(bc -l <<< "scale=5; 1000*($t2-$t1)/$COUNT")ms"
    echo "+commands: $((t3-t2))s $(bc -l <<< "scale=5; 1000*($t3-$t2)/$COUNT")ms"
    echo "command-v: $((t4-t3))s $(bc -l <<< "scale=5; 1000*($t4-$t3)/$COUNT")ms"
} |column -t
❯ COUNT=100000 /tmp/commands
command      total  average
+commandsF:  1s     .01000ms
command-vF:  72s    .72000ms
+commands:   0s     0ms
command-v:   49s    .49000ms

Note that this measuring is very inaccurate since I call date to measure points in time. I also tried them inside and outside functions. In this case any inaccuracy does not matter. (( $+commands[who] )) is leagues faster than [[ -z "$(command -v who)" ]]

@z0rc
Copy link

z0rc commented Oct 16, 2024

@Syphdias instead of date you can use $EPOCHREALTIME from zsh/datetime module https://zsh.sourceforge.io/Doc/Release/Zsh-Modules.html#index-EPOCHREALTIME.

@romkatv
Copy link
Owner

romkatv commented Oct 16, 2024

My favorite recipe for quick-n-dirty benchmarks:

time ( repeat 100000 (( $+commands[who] )) )

Adjust the repeat count to get the total run time of at least 100ms.

@Syphdias
Copy link
Contributor

My favorite recipe for quick-n-dirty benchmarks:

I used that first with {} instead of () but wanted to make sure I tested in fair conditions in case I missed something. I thought $() would always fork but I read somewhere that is not the case for simple commands, but I did not trust that. Maybe this optimisation was only for scripts or something.

Any how, as expected, searching through a variable is faster than calling a program/function…

@visigoth
Copy link
Author

How did you benchmark that the new comparison is faster? And how big is commands for you?

i used hyperfine to run the command inside a terminal. i don't have the command handy in my terminal history at the moment, though. for reference echo $#commands prints 4009.

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.

4 participants