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

tab-completing formula names is very slow #20

Closed
adamv opened this issue Apr 4, 2016 · 23 comments
Closed

tab-completing formula names is very slow #20

adamv opened this issue Apr 4, 2016 · 23 comments

Comments

@adamv
Copy link
Contributor

adamv commented Apr 4, 2016

Please follow the general troubleshooting steps first:

  • [*] Ran brew update and retried your prior step?
  • [*] Ran brew doctor, fixed as many issues as possible and retried your prior step?

Bug reports:

tab-completion of formula names is very slow (many seconds), after updating to brew 0.9.9. I only have the core tap installed.

I'm actually unsure if these completions were already slow before, as I hadn't triggered one for a while before this latest update.

@tdsmith
Copy link
Contributor

tdsmith commented Apr 4, 2016

They were already unusably slow but I think maybe it got even worse?

@tdsmith
Copy link
Contributor

tdsmith commented Apr 5, 2016

Oh, it's scanning Library/Formulas which is a symlink to the core tap now, and then it scans all the taps, so it's ~twice as bad as it used to be.

@tdsmith
Copy link
Contributor

tdsmith commented Apr 5, 2016

also i'm pretty sure the algorithm used for taps is quadratic in the number of formulas

tdsmith added a commit to tdsmith/brew that referenced this issue Apr 5, 2016
tdsmith added a commit that referenced this issue Apr 5, 2016
@pavelrad
Copy link

This was very fast before all core formulae were moved to a separate core tap. Now it's very slow. Just compare to brew-cask completion, which takes just a moment to complete formula name.

@MikeMcQuaid
Copy link
Member

@pavelrad Casks are also in a tap so there's a bug here we'll need help fixing.

@pavelrad
Copy link

Why this issue is closed? It's definitely not fixed yet.

@MikeMcQuaid
Copy link
Member

@pavelrad Because it's less slow than it was.

@MikeMcQuaid MikeMcQuaid reopened this May 18, 2016
@tdsmith
Copy link
Contributor

tdsmith commented May 20, 2016

In my subjective experience it's now much faster than it was before the split! I don't know why we're seeing different things.

@metacollin
Copy link

No, it is not fixed at all.

It used to take a couple hundred milliseconds.

It now takes 3-5 seconds of one of my 3.2GHz Xeon cores maxed out at 100% utilization. Performing tab completion in brew actually makes my fans spin up.

I am not sure what the exact mechanism is yet, but the issue is homebrew core being a tap. That's what is causing the unacceptably poor tab completion performance.

It's not the aliases, nor is it having to look through stuff twice (Formula, AND the tap). It's just the fact that is there at all. If you remove the Formula alias in /usr/local/Library/, as well as the Aliases alias, tap completion performance is not improved even slightly. It's still dog slow.

If you move the Formula directory out of the homebrew tap and anywhere else, then create a new symlink to it in the /usr/local/Library directory, homebrew's old 200ms tab completion performance immediately returns. Or just putting it back in its old spot works too. Whatever is going on, it has nothing to do with the number of formula, and everything to do with simply being in the taps folder.

@metacollin
Copy link

metacollin commented May 20, 2016

I figured out the problem. We're getting differing results because some of us have an old completion script for homebrew that is getting loaded in addition to the new one.

Everyone, check your $(brew --prefix)/etc/bash_completion.d/ directory for any other files starting with 'brew'. There should be exactly one file, which is called simply 'brew'. I found I had a 'brew_bash_completion.sh' and 'brew_bash_completion.sh.default' AND 'brew_bash_completion.sh.default.save' in that directory. They're leftovers from brew-legacy. Or something. I don't know where that .save came from. Maybe I caused it, or maybe other people have it, just check for any suspicious files with brew in the name. Leave only the file named 'brew'.

One removed, the speed issues immediately go away while all completions continue to work. You probably will want to open a fresh shell to test.

And @tdsmith is right, the brew bash completion script appears to have been optimized, so anyone who had an unpolluted bash_completion.d directory (bash_completion automatically loads everything in that directory) would see exactly what @tdsmith sees: a slightly faster brew tab completion.

The old completion scripts do some things that result in very inefficient completion if there are a large number of formulae in a tap. Before the split, this was fine, as there would not be a huge number of formulae there. Now however, having 3500 Formulae in the taps folder makes the old completion script basically unusable.

I have no idea how or why those old files are there, but I think its just a case of a tiny detail getting overlooked.

Man, I am so happy to have my completions back. Fast as ever now!

@pavelrad
Copy link

Great! Removing 'brew_bash_completion.sh' really helped with this issue, thank you, @metacollin.

@MikeMcQuaid
Copy link
Member

@pavelrad @metacollin Glad to hear it. We could/should perhaps have Homebrew remove them for you.

@pavelrad
Copy link

Yes, that would be nice to have some automatic cleanup of these old scripts.

@metacollin
Copy link

Thirded.

@UniqMartin
Copy link
Contributor

We could/should perhaps have Homebrew remove them for you.

Any idea where to put this? I could imagine this being a (very lightweight) new check in brew doctor, but in this case we could only suggest the removal and not perform the removal automatically. (Since we're always asking for brew doctor output for troubleshooting, this might be sufficient.)

@MikeMcQuaid
Copy link
Member

@UniqMartin Feels like the sort of cleanup update-report.rb does?

@UniqMartin
Copy link
Contributor

Feels like the sort of cleanup update-report.rb does?

Didn't think of it, but this sounds like the right place. I'm still a bit hesitant to automatically delete stuff in etc, even if we're pretty sure the files are created by Homebrew. I think so far we've refrained from doing something like this. (There might be a tiny minority of users who have customized etc/bash_completion.d/brew_bash_completion.sh and would be unhappy about it being silently wiped from their disk. Just wanted to mention this; I'm personally not very concerned that's a significant issue—unlike the tab completion slowness.)

@MikeMcQuaid
Copy link
Member

I think so far we've refrained from doing something like this.

I think we've actually taken more extreme measures previously:

brew/Library/brew.rb

Lines 81 to 84 in 0123e04

# Uninstall old brew-cask if it's still around; we just use the tap now.
if cmd == "cask" && (HOMEBREW_CELLAR/"brew-cask").exist?
system(HOMEBREW_BREW_FILE, "uninstall", "--force", "brew-cask")
end

@tdsmith
Copy link
Contributor

tdsmith commented May 20, 2016

There's also the mv foo foo.bak approach. 👍 for auto-cleanup one way or another.

Probably paranoid, but would we want to replace it with a zero-length file in case someone's shell profile sources it without checking for existence so we don't break their terminal?

@UniqMartin
Copy link
Contributor

There's also the mv foo foo.bak approach. 👍 for auto-cleanup one way or another.

We'd have to move it out of the directory, but I generally like the idea of moving files instead of wiping them. The question here would be where to move them to? Do we want to introduce the equivalent of lost+found for Homebrew and where would that live?

Probably paranoid, but would we want to replace it with a zero-length file in case someone's shell profile sources it without checking for existence so we don't break their terminal?

I think in this case it's better to break the profile, so that it can be fixed. The error message will be sufficiently descriptive and if people come to us with that error, we'll know what to do. If too many issues are created after doing this (I hope not), we can still create a dummy file (with a comment that it's obsoleted by brew) there to avoid being swamped.

@metacollin
Copy link

metacollin commented May 21, 2016

It's worth noting that uninstalling bash completion (either the actual package or from within homebrew) removes a large number of scripts from bash_completion.d/ and overwrites modifications etc. upon installation. bash_completion's documentation explicitly states that user modifications to installed completion scripts in that directory will not persist over upgrades, installations, or uninstalls. In my opinion, brew's expected behavior would be to overwrite any files in that folder that it created (even if it was quite a while ago) upon an update if needed. There is a separate folder for user modified or user created completions, $(brew --prefix)/etc/profile.d/.

But, I think that someone might sourcing that file directly without doing file test is actually a pretty valid concern, I've seen a lot of people who do some pretty terrible things in the name of 'ricing' their shell, like aliasing cp to call rsync, or having which python return a bash script that would return a path instead of a path, and I'm sure there is at least one unfortunate bastard out there who is sourcing this homebrew file without checking if it exists first, and he or she is of course doing it in their system-wide bash profile no less. Probably on a production machine used to keep baby kittens alive or something.

But, noting what I said in the first paragraph, we could simply overwrite the file with an empty one, thus solving the problem without causing Mr. or Mrs. Unfortunate Bastard (and their baby kittens) a very bad day.

An even safer option might be to only do that if it is unchanged from the original homebrew-legacy script (this would need to be a manual check I think - but it could just pull it in from github or a gist to do the check), and if there are changes, move it elsewhere, symlink it into bash_completion.d/, track it with git in its new home (since a .git folder in bash_completions.d/ would be no bueño), and emit the familiar 'git stash pop' warning about user changes. But, I also feel that homebrew would be completely within its bounds to overwrite that file with an empty one.

@apjanke
Copy link
Contributor

apjanke commented May 22, 2016

Am out & about, so will give more feedback later, but one quick thing to be aware of: "bash completion" can mean two things in Homebrew land. There's brew's own support for bash completion, and there's the bash-completion formula, which is a separate project that provides completions for many commands (but not brew). These two things are independent. "Uninstalling bash completion" and the $(brew --prefix)/etc/profile.d sounds like you're referring to the bash-completion formula stuff. Brew's own bash completion support gets installed directly to /usr/local/etc/bash_completion.d/brew, should be independent of the bash-completion formula, and there is no support for uninstallation of it.

(There's a similar issue with zsh-completion vs "zsh completion". That was more complicated because zsh-completion did provide completions for brew, and then removed it when brew started shipping its own zsh completion support.)

The stuff about leaving user modifications alone to "bash_completion" might be from bash-completion's own doco. AFAIK, brew's bash completion does not support user modification at all. And, as a separate issue, the way we install bash-completion might be messing with their support for unmolested user modifications, but I don't think that's the problem this GitHub issue is addressing.

Will give this a close reading and feedback later tonight.

@MikeMcQuaid
Copy link
Member

This has been improved as much as it will be for now. If this is still broken for you: we'll accept PRs for this but we're not actively working on it at this time.

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants