rcaloras / bash-preexec

⚡ preexec and precmd functions for Bash just like Zsh.
MIT License
862 stars 94 forks source link

Avoid checking PATH for functions #161

Closed agriffis closed 1 month ago

agriffis commented 1 month ago

Searching PATH for functions is slow and unnecessary...

agriffis commented 1 month ago

I force-pushed this a couple times just to clean it up.

akinomyoga commented 1 month ago

Searching PATH for functions is slow

Have you made a benchmark? The command substitution $(...) causes a fork of the process which is usually even slower than the PATH search. This PR makes it 100 times "slower" (instead of faster). This is the result in my system (GNU/Linux):

$ time for i in {000..999}; do [[ $(PATH= type -t _nonexistent) == function ]]; done

real    0m2.894s
user    0m1.010s
sys     0m1.898s
$ time for i in {000..999}; do [[ $(PATH= type -t _comp_initialize) == function ]]; done

real    0m2.954s
user    0m1.063s
sys     0m1.954s
$ time for i in {000..999}; do type -t _nonexistent >/dev/null; done

real    0m0.034s
user    0m0.019s
sys     0m0.014s
$ time for i in {000..999}; do type -t _comp_initialize >/dev/null; done

real    0m0.015s
user    0m0.012s
sys     0m0.004s

and unnecessary...

Despite the name "precmd_function", an entry in the array could be an executable file. Users have been able to specify executable command names, and they have worked all the time until now. This PR breaks the compatibility.

In addition, for the users who specify only function names in precmd_function, trying to skip the PATH wouldn't make any difference in the performance because of the search order. Bash searches the function name first, and only when it doesn't match any function name, it performs the search in PATH. The PATH search would never happen as far as the specified names are function names.

agriffis commented 1 month ago

Hey there, thanks for looking at this.

I did not benchmark. The motivator for this PR is a badly-behaved system where type -t for non-existent functions takes nearly half a second. My apologies for not realizing this would affect the common case.

FWIW, command substitutions typically don't fork in bash unless they call an external command. Bash has some internal optimizations to avoid forking for builtins, for example. You can see this with:

echo $$
1234

echo $(echo $$)
1234

echo $(bash -c 'echo $$')
2345

That's a great point about searching functions before PATH, though! I agree I opened this without sufficient research and experimentation!

akinomyoga commented 1 month ago

FWIW, command substitutions typically don't fork in bash unless they call an external command. Bash has some internal optimizations to avoid forking for builtins, for example. You can see this with:

echo $$
1234

echo $(echo $$)
1234

echo $(bash -c 'echo $$')
2345

No, you misunderstand $$. $$ always returns the process ID of the main shell. To check the process ID of the current process, you need to reference $BASHPID. ksh performs such an optimization, but Bash doesn't do that except for $(< file) in recent versions. Please read Bash Reference Manual:

You should check the results of the following commands:

$ echo "$$:$BASHPID"
1997853:1997853
$ echo $(echo "$$:$BASHPID")
1997853:1998890
agriffis commented 1 month ago

Oh, thanks!