-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Improve complete.bash #113
base: main
Are you sure you want to change the base?
Conversation
# usage: _tldr_get_files [architecture] [semi-completed word to search] | ||
_tldr_get_files() { | ||
local ret | ||
local files="$(find $HOME/.tldrc/tldr/pages/$1 -name '*.md' -exec basename {} .md \;)" | ||
|
||
IFS=$'\n\t' | ||
for f in $files; do | ||
echo $f | ||
done | ||
find "$HOME"/.tldrc/tldr/pages/"$1" -name "$2"'*.md' -exec basename -s .md {} + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend the reviewers to test if find ... -name '*.md' ...
is better, or is find ... -name "$2"'*.md' ...
.
Comparing the performance of _tldr_complete
function using time
command does not help, since both methods perform almost the same.
Using the following commands, I was able to determine that after applying this change, the bottleneck is the find
command:
$ word="he"
$ cmpl=`find $HOME/.tldrc/tldr/pages/linux/ -name "$word"'*.md' -exec basename -s '.md' {} + && find $HOME/.tldrc/tldr/pages/common/ -name "$word"'*.md' -exec basename -s '.md' {} +`; cmpl_sorted_n_uniq=`printf "%s" "$cmpl" | sort | uniq`
$ compgen -W "$cmpl_sorted_n_uniq" -- "$word" | pv -lr > /dev/null
$ ( find $HOME/.tldrc/tldr/pages/linux/ -name "$word"'*.md' -exec basename -s '.md' {} + && find $HOME/.tldrc/tldr/pages/common/ -name "$word"'*.md' -exec basename -s '.md' {} + ) | pv -rl >/dev/null
The results will be in this form:
[compgen's speed]
[find's speed]
In the other case (using only -name '*.md'
in find command), I found compgen
was the bottleneck instead of find
; using the following commands:
$ word="he"
$ cmpl=`find $HOME/.tldrc/tldr/pages/linux/ -name '*.md' -exec basename -s '.md' {} + && find $HOME/.tldrc/tldr/pages/common/ -name '*.md' -exec basename -s '.md' {} +`; cmpl_sorted_n_uniq=`printf "%s" "$cmpl" | sort | uniq`
$ compgen -W "$cmpl_sorted_n_uniq" -- "$word" | pv -lr > /dev/null
$ ( find $HOME/.tldrc/tldr/pages/linux/ -name '*.md' -exec basename -s '.md' {} + && find $HOME/.tldrc/tldr/pages/common/ -name '*.md' -exec basename -s '.md' {} + ) | pv -rl >/dev/null
The result will be in the same form as described above.
The results I got:
- with
find ... -name '*.md' ...
:compgen
's speed: 1.69k/sfind
's speed: 241k/s
- with
find ... -name "$2"'*.md' ...
:compgen
's speed: 255k/sfind
's speed: 1.48k/s
Both methods used about 0.05 secs on my system (using time _tldr_complete
).
Thanks for the quick fix @lakshayrohila ! |
What does it do?
Improves
complete.bash
by:_tldr_get_files()
, which does nothing but re-echo
es the same output.| sort | uniq
calls._tldr_get_files()
now only returns the matching results, instead of returning thousands of lines each time.--vers
, which it should autocomplete. This version would autocomplete in that case totldr --version
.--verbose
); this version adds them.Why the change?
The autocomplete function almost freezes (on my system) for almost 20 secs (I am using it in bash). This is due to the above reasons. This version reduces that time to almost instantaneous.
How can this be tested?
Install the autocomplete file just as is described in this repo's
README.md
, and try to use completion (with tab key press) in bash.Where to start code review?
I've already mentioned the major changes in the What does it do? section.
Questions?
N/A
Checklist
make
and tested the change in an active installation withsudo make install
.Notes for (2) : Since this is not a change in the code for the actual
tldr
program, I have not run (2); but rather tested the change with the steps I mentioned in How can this be tested? section.