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-it completions performance improvement #1989

Merged

Conversation

tsiflimagas
Copy link
Contributor

@tsiflimagas tsiflimagas commented Nov 30, 2021

Description

I went on and removed the usage of loops from the bash-it completions, and arranged the functions to essentially perform the same acts, but process the file lists after having accumulated them, instead of performing tests etc for each and every one of the files. While I'm at it, I also double quoted some variables to avoid having the interpreter spend time trying to perform splitting on them (a bit of nitpick, but why not), made a couple of variables local, since they seemed to have gotten unnoticed and replaced a usage of sed with the pattern substitution parameter of parameter expansion. I was also thinking of eliminating the usage of pipes by using process substitution, thus eliminating spawned subshells and having the rest of process substitution performance benefits, but I don't know if this would go too far and make the code a bit hard to read with all the nested parentheses and redirections.

Motivation and Context

As mentioned in the linked issue, the performance of bash-it completions generation, end especially for the `enable` option is rather poor, which is a significant problem for me, as I relatively regularly use them. This PR massively improves this aspect, to the point where I'd say completions for `bash-it` are now very fast.

Closes #1988

How Has This Been Tested?

Apart from using the very completions to notice the performance difference and passing the tests, I kind of benchmarked them by exporting the `__check_completion` function from the completions test, and running tests like `time __check_completion 'bash-it enable completion docker'`. I ran tests on my phone, laptop and gcp server which use Android 12, Manjaro 21.2.0 and Ubuntu 21.10 respectively. On my phone, from and average duration of 4s, it dropped to ≈100ms when files aren't cached and ≈60ms after they are, on my dual core laptop from an average of ≈1s it dropped to ≈80ms when files aren't cached and ≈25ms when they are, and on my 8 core server from ≈700ms to ≈10ms (always). In every case, very fast I think.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@tsiflimagas tsiflimagas changed the title Bashit completions performance improvement Bash-it completions performance improvement Nov 30, 2021
@NoahGorny
Copy link
Member

Hi @tsiflimagas
I will try to take a look today, thanks for your contribution ❤️

@tsiflimagas
Copy link
Contributor Author

No problem!😄
Damn it, it seems that this implementation doesn't work on Macos. I'm not sure why, but I suspect it could be because of it having BSD grep, instead of GNU one (though I looked into some documentation and it seems that it supports the options I used). Also, build-docs test seems to have failed due to inability to install some python package (I guess it's unrelated to my changes). Anyway, I figured out I could replace external programs with bash features more extensively, which I guess will improve portability and improves speed a bit further. I hope it fixes execution on Macos. It just looks ugly that I repeat variable names like that, so please tell me if you think of something better.

@gaelicWizard
Copy link
Contributor

Awesome! Thanks for putting this together 😃

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is much faster now- great work!

@NoahGorny NoahGorny merged commit 2e968c4 into Bash-it:master Jan 7, 2022
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.

Bash-it enable completions performance issue
3 participants