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

Avoid checking PATH for functions #161

Closed
wants to merge 1 commit into from
Closed

Conversation

agriffis
Copy link

Searching PATH for functions is slow and unnecessary...

agriffis added a commit to agriffis/skel that referenced this pull request Jul 25, 2024
@agriffis
Copy link
Author

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

@akinomyoga
Copy link
Contributor

akinomyoga commented Jul 25, 2024

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
Copy link
Author

agriffis commented Jul 25, 2024

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!

@agriffis agriffis closed this Jul 25, 2024
@akinomyoga
Copy link
Contributor

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
Copy link
Author

Oh, thanks!

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.

2 participants