Improve zsh completion performance #7766
Conversation
There was a problem hiding this comment.
Makes sense to me so far! Haven't reviewed #7767 yet but it may be worth thinking about that, too.
@MikeMcQuaid I'm not sure what that PR is trying to do and how it relates to this one, but I'm open to any changes you want to suggest. |
@marlonrichert It's a different approach. Try it out (it's been committed to |
|
@MikeMcQuaid OK, thanks. I’ll try it out. |
|
@MikeMcQuaid I just tested So, I hope you will still consider merging this. #7767 has not made it obsolete. |
|
|
||
| comp_cachename=brew_formulae | ||
| if _cache_invalid $comp_cachename; then | ||
| list=( $(brew search) ) |
There was a problem hiding this comment.
What about having the output of this be cached in HOMEBREW_CACHE like all_commands_list.txt on brew update instead? That would enable it to be used in Bash completion too. CC @alebcay for thoughts.
There was a problem hiding this comment.
I think ideally the formula list would be generated with Ruby and cached in HOMEBREW_CACHE and then consumed by whatever completions we'd like. We could still store a copy of this list in the Zsh completions cache if that makes the completion execution marginally faster.
There was a problem hiding this comment.
__brew_installed_formulae and __brew_installed_casks could also be cached. Those are quite slow, too.
There was a problem hiding this comment.
I think ideally the formula list would be generated with Ruby and cached in
HOMEBREW_CACHEand then consumed by whatever completions we'd like. We could still store a copy of this list in the Zsh completions cache if that makes the completion execution marginally faster.
Homebrew's Zsh completion code could just read the completions straight out of HOMEBREW_CACHE, instead of calling brew or using Zsh's own caching mechanism. I need to test it, but I think it should be just as fast as using Zsh's completion cache.
There was a problem hiding this comment.
Homebrew's Zsh completion code could just read the completions straight out of HOMEBREW_CACHE
I believe this is what we're doing right now after #7767 was merged. I left the existing completion cache code in since it would still be useful for the other Zsh completions (formulae and casks), and wouldn't hurt anything.
There was a problem hiding this comment.
I'd rather not make the
cmd/update.shchange given the discussion above, sorry. I'm open to other parts of this PR being merged. Whencmd/update.shfails: no-one can update Homebrew so I'm extremely reluctant to make changes there that aren't 100% necessary.
OK, I'll remove the cmd/update.sh changes from this PR.
There was a problem hiding this comment.
Thanks! Please roll Library/Homebrew/utils/update-completions.zsh back into completions/zsh/_brew too. After that, though, I'm not sure what's actually changing here?
There was a problem hiding this comment.
What will be left are three things that speed up Zsh completions:
- Enable Zsh completion caching for
brewarguments by default. (Right now, it’s disabled by default.) - Use
$(<file)instead of$(cat file). (The former is faster than the latter, because it does not spawn a new process, which the latter does.) - Don’t call
_cache_invalidbefore calling_retrieve_cache. (The latter already calls the former. No need to call it twice.)
There was a problem hiding this comment.
@MikeMcQuaid I made the changes. Please review.
* Move main completion cache handling to `brew update`. * Enable completion caching by default.
brew update.brew stylewith your changes locally?brew testswith your changes locally?Improve perceived zsh completion performance by
brew updateis being run, instead of doing it when the user tries to get a completion, since the user is expecting to wait duringbrew updateanyway, and byI have not written tests for this, because I have no idea how to test this. I looked at other PRs for Zsh completions, but they don't seem to have included tests either. I guess no one really know how to test them.