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

[bash-completion] Optimize startup + two minor fixes #2246

Merged
merged 4 commits into from Nov 12, 2020

Conversation

liskin
Copy link
Contributor

@liskin liskin commented Nov 11, 2020

Commit d4ad4a2 slowed loading of completion.bash significantly (on my
laptop from 10 ms to 30 ms), then 54891d1 improved that (to 20 ms) but
it still stands out as the heavy part of my .bashrc. Furthermore, an
omission in d4ad4a2 caused meaningless "complete _a" and "complete _v"
completions to be defined.

This pull request fixes that mistake, refactors the code to prevent
making it again, and most importantly optimizes
__fzf_orig_completion_filter so that loading completion.bash is as fast
as it was before d4ad4a2, that is under 10 ms on my laptop.

(I tried not to use any features not present in bash 3.2, but I didn't
test it with bash 3.2. Can someone try this on MacOS please?)

This doesn't look right:

    $ complete | grep ' _.$'
    complete _a
    complete _v

The __fzf_orig_completion_filter invocation in _fzf_setup_completion
needs the /-F/ filter, just like all the other invocations.

Fixes: d4ad4a2 ("[bash-completion] Fix default alias/variable completion")
This prevents mistakes like the one fixed by the previous commit, and
also speeds bash startup a tiny bit:

before:

    $ HISTFILE=/tmp/bashhist hyperfine 'bash --rcfile shell/completion.bash -i'
    Benchmark junegunn#1: bash --rcfile shell/completion.bash -i
      Time (mean ± σ):      22.4 ms ±   0.6 ms    [User: 28.7 ms, System: 7.8 ms]
      Range (min … max):    21.7 ms …  25.2 ms    123 runs

after:

    $ HISTFILE=/tmp/bashhist hyperfine 'bash --rcfile shell/completion.bash -i'
    Benchmark junegunn#1: bash --rcfile shell/completion.bash -i
      Time (mean ± σ):      21.2 ms ±   0.3 ms    [User: 24.9 ms, System: 6.4 ms]
      Range (min … max):    20.7 ms …  23.3 ms    132 runs
shell/completion.bash Outdated Show resolved Hide resolved
Commit d4ad4a2 slowed loading of completion.bash significantly (on my
laptop from 10 ms to 30 ms), then 54891d1 improved that (to 20 ms) but
it still stands out as the heavy part of my .bashrc.

Rewriting __fzf_orig_completion_filter to pure bash without forking to
sed/awk brings this back under 10 ms.

before:

    $ HISTFILE=/tmp/bashhist hyperfine 'bash --rcfile shell/completion.bash -i'
    Benchmark junegunn#1: bash --rcfile shell/completion.bash -i
      Time (mean ± σ):      21.2 ms ±   0.3 ms    [User: 24.9 ms, System: 6.4 ms]
      Range (min … max):    20.7 ms …  23.3 ms    132 runs

after:

    $ HISTFILE=/tmp/bashhist hyperfine 'bash --rcfile shell/completion.bash -i'
    Benchmark junegunn#1: bash --rcfile shell/completion.bash -i
      Time (mean ± σ):       9.6 ms ±   0.3 ms    [User: 8.0 ms, System: 2.2 ms]
      Range (min … max):     9.3 ms …  11.4 ms    298 runs

Fixes: d4ad4a2 ("[bash-completion] Fix default alias/variable completion")
Fixes: 54891d1 ("[bash-completion] Minor optimization")
@junegunn
Copy link
Owner

Thanks Tomáš, I really like what you did here. Bravo.

@junegunn junegunn merged commit 75b8cca into junegunn:master Nov 12, 2020
1 check was pending
@liskin
Copy link
Contributor Author

liskin commented Nov 12, 2020

It's a pleasure. Thank you for maintaining this excellent piece of software!

@wingpin6
Copy link

wingpin6 commented Nov 13, 2020

Hi guys, thanks for your efforts on this program and improving it.

Unfortunately for me, a commit within this PR has caused my terminal to crash when trying to tab. I've identified commit ef2c29d through git bisect as the offending commit.

For reference I am currently using:
GNU bash, version 5.0.17(1)-release (x86_64-pc-linux-gnu)
on Ubuntu 20.04
Terminal choice does not seem to matter; I crash on both Alacritty (my daily driver) and Gnome Terminal.

I hang then crash when I use any program (such as cd, ls, cp) and then pressing tab.

@liskin
Copy link
Contributor Author

liskin commented Nov 13, 2020

@wingpin6 Could you please try running a nested bash in your terminal and reproducing this in it? This way the terminal won't terminate when the bash terminates and we might see an error message if there is any.

@liskin
Copy link
Contributor Author

liskin commented Nov 13, 2020

@wingpin6 I tried whether I can reproduce it using ubuntu:20.04 in docker and I can't, so it seems this is in some way related to your environment or other bits of your bashrc. :-/

@wingpin6
Copy link

wingpin6 commented Nov 13, 2020

@liskin Thank you for the tip; after doing nested bash --norc I found that

  • sourcing only .bashrc did not crash
  • sourcing only .fzf.bash did not crash
  • However, sourcing .bashrc and .fzf.bash caused it to crash

To test it, the only line I have in .bashrc is
[ -f ~/.fzf.bash ] && source ~/.fzf.bash
and .fzf.bash is unaltered
.fzf.bash contents:

# Setup fzf
# ---------
if [[ ! "$PATH" == */home/user/.fzf/bin* ]]; then
  export PATH="${PATH:+${PATH}:}/home/user/.fzf/bin"
fi

# Auto-completion
# ---------------
[[ $- == *i* ]] && source "/home/user/.fzf/shell/completion.bash" 2> /dev/null

# Key bindings
# ------------
source "/home/user/.fzf/shell/key-bindings.bash"
$ bash --norc
bash-5.0$ source .fzf.bash
bash-5.0$ source .bashrc
bash-5.0$ ls Segmentation fault (core dumped)

So the error it outputted was Segmentation fault.

I looked deeper into this issue, and was able to fix it if I removed
[ -f ~/.fzf.bash ] && source ~/.fzf.bash
from my .bashrc

The error did not occur if I did not use .bashrc as my default bash config file, and then source .bashrc and .fzf.bash within that new config file separately.

Let me know if you need more info or if you can't replicate that.

@liskin liskin deleted the bash-completion branch November 13, 2020 15:30
@liskin
Copy link
Contributor Author

liskin commented Nov 13, 2020

@wingpin6 Segmentation fault seems to be a bug in bash, provided it comes from bash and not from something else.

Could you get a stack trace from the core dump? I'm not sure if Ubuntu has systemd-coredump, but if yes, then this should be as easy as running coredumpctl info and sending the output. If you could also apt install bash-dbgsym (https://wiki.ubuntu.com/Debug%20Symbol%20Packages) and then do coredumpctl debug and then type btinto gdb, that would be wonderful.

If there's no systemd-coredump in Ubuntu, instead of running bash directly, run gdb -ex run --args bash and reproduce the crash and then when it crashes, get stack trace by typing bt.

I also looked at recent changes in bash in Ubuntu and it seems that this may possibly help: liskin@6b4ab0a, although that is really a wild guess, nothing else.

@liskin
Copy link
Contributor Author

liskin commented Nov 13, 2020

Oh, and it seems I can replicate that now. The key is sourcing completion.bash twice!
No need to debug it then, I'll take care of it!

@liskin
Copy link
Contributor Author

liskin commented Nov 13, 2020

@wingpin6 Fix in #2250. Thanks for reporting this and sorry for breaking your setup.

(You might, however, consider sourcing completion.bash just once in the future, as that will be faster. :-))

@wingpin6
Copy link

Thank you for the quick fix!

I agree this case is most likely uncommon. I have a personal config file that I use on all my machines and a work config file for my work machine. I source both of them with work config overriding anything I defined in my personal config, with some overlaps.

I happened to source .fzf.bash in both of them; not sure how that got into my work config file. But great to find out that I can remove that line now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants