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

prefix.sh: support API mode #14622

Merged
merged 1 commit into from Feb 15, 2023
Merged

prefix.sh: support API mode #14622

merged 1 commit into from Feb 15, 2023

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Feb 14, 2023

I noticed we already do this in formulae.sh:

# HOMEBREW_CACHE is set by brew.sh
# shellcheck disable=SC2154
if [[ -z "${HOMEBREW_NO_INSTALL_FROM_API}" &&
-f "${HOMEBREW_CACHE}/api/formula.json" ]]
then
local api_formulae
api_formulae="$(ruby -e "require 'json'; JSON.parse(File.read('${HOMEBREW_CACHE}/api/formula.json')).each { |f| puts f['name'] }" 2>/dev/null)"
formulae="$(echo -e "${formulae}\n${api_formulae}" | sort -uf | grep .)"
fi

So it makes sense to do in prefix.sh if we want to keep the speed advantages there too.

If the JSON file is corrupt or missing, it will still fallback to prefix.rb as before where a download will occur.

@BrewTestBot
Copy link
Member

Review period will end on 2023-02-15 at 04:35:14 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 14, 2023
@Bo98 Bo98 added the install from api Relates to API installs label Feb 14, 2023
-z "${HOMEBREW_NO_INSTALL_FROM_API}" &&
-f "${HOMEBREW_CACHE}/api/formula.json" ]]
then
formula_exists="$(ruby -e "require 'json'; puts 1 if JSON.parse(File.read('${HOMEBREW_CACHE}/api/formula.json')).any? { |f| f['name'] == '${formula}' }" 2>/dev/null)"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a heredoc for readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

-f "${HOMEBREW_CACHE}/api/formula.json" ]]
then
formula_exists="$(
ruby -rjson <<RUBY 2>/dev/null
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we're guaranteed a ruby here, are we? Not a dealbreaker though, since I suppose we can still fallback to --prefix.rb.

Copy link
Member

Choose a reason for hiding this comment

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

But, if so (or even alternatively to avoid having to fire up ruby), perhaps we can consider doing

[[ -h "${HOMEBREW_PREFIX}/opt/${formula}" ]]

somewhere earlier to handle the case where the formula is installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not, though that but would also affect brew formulae. And yes, if it's not present it will fall back.

Maybe could do what _create_lock does and fallback to Python and then something else.

In fact the consequences are worse for brew formulae as it breaks shell completion etc so it may be worth improving that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe just hardcode a PATH to portable-ruby vendor directory somewhere. System Ruby 2.6 outside of macOS is pretty much dead at this point so there's usually a portable Ruby installed.

Copy link
Member Author

@Bo98 Bo98 Feb 14, 2023

Choose a reason for hiding this comment

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

But, if so (or even alternatively to avoid having to fire up ruby), perhaps we can consider doing

[[ -h "${HOMEBREW_PREFIX}/opt/${formula}" ]]

somewhere earlier to handle the case where the formula is installed.

Oops missed this somehow. Decent idea actually since that'll be even faster than parsing a large JSON. The only difference is the deleted formulae case, which could be argued as an improvement.

Doesn't fix brew formulae though.

Copy link
Member

Choose a reason for hiding this comment

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

We're not, though that but would also affect brew formulae.

I think it'd be nice to have a shared .sh file with a function that both of these can use to do this JSON operation?

Or maybe just hardcode a PATH to portable-ruby vendor directory somewhere.

Let's not unless we get a bug report.

Copy link
Member

Choose a reason for hiding this comment

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

Or, better still: extract all the Ruby code into a .rb file.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Fine with me but would be nice to share some code here. Can you use hyperfine to demonstrate the speedup, too? Want to make sure this actually meaningfully improves performance for the amount of somewhat gnarly code we're adding here.

-f "${HOMEBREW_CACHE}/api/formula.json" ]]
then
formula_exists="$(
ruby -rjson <<RUBY 2>/dev/null
Copy link
Member

Choose a reason for hiding this comment

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

Or, better still: extract all the Ruby code into a .rb file.

@Bo98
Copy link
Member Author

Bo98 commented Feb 14, 2023

Worst case before this PR - though this is connection dependent so may be several seconds for some people.
I say "worst" case but this happens once every 30 minutes (by default) so isn't rare.

$ hyperfine --warmup 3 "HOMEBREW_API_AUTO_UPDATE_SECS=0 brew --prefix hello"
Benchmark 1: HOMEBREW_API_AUTO_UPDATE_SECS=0 brew --prefix hello
  Time (mean ± σ):      2.416 s ±  0.162 s    [User: 1.542 s, System: 0.690 s]
  Range (min … max):    2.242 s …  2.844 s    10 runs

Best case before this PR:

$ hyperfine --warmup 3 "HOMEBREW_NO_AUTO_UPDATE=1 brew --prefix hello"
Benchmark 1: HOMEBREW_NO_AUTO_UPDATE=1 brew --prefix hello
  Time (mean ± σ):      1.831 s ±  0.083 s    [User: 1.259 s, System: 0.503 s]
  Range (min … max):    1.748 s …  2.011 s    10 runs

After this PR:

$ hyperfine --warmup 3 "brew --prefix hello"
Benchmark 1: brew --prefix hello
  Time (mean ± σ):     787.8 ms ±  13.3 ms    [User: 665.9 ms, System: 106.0 ms]
  Range (min … max):   768.8 ms … 808.1 ms    10 runs

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 14, 2023
@MikeMcQuaid
Copy link
Member

@Bo98 performance looks great!

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 14, 2023
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@MikeMcQuaid
Copy link
Member

@Bo98 another option that occurred to me for both places would be that we could consider having something in Ruby-land dump to a .txt file in a format that can be more easily handled by Bash. That would be able to drop the ruby call both in here and formulae.sh which seems likely to be a bunch faster still.

@Bo98
Copy link
Member Author

Bo98 commented Feb 14, 2023

Yes, that's a very good idea. The remaining bottleneck compared to non-API is also the JSON parsing.

@MikeMcQuaid
Copy link
Member

@Bo98 and just shelling out to Ruby (which takes 0.13s on my M1 Max to do puts)

@carlocab
Copy link
Member

@Bo98 another option that occurred to me for both places would be that we could consider having something in Ruby-land dump to a .txt file in a format that can be more easily handled by Bash. That would be able to drop the ruby call both in here and formulae.sh which seems likely to be a bunch faster still.

I've been thinking about this too. But I think I picked it up from an idea @Rylan12 mentioned in another PR.

@Rylan12
Copy link
Member

Rylan12 commented Feb 14, 2023

@Bo98 another option that occurred to me for both places would be that we could consider having something in Ruby-land dump to a .txt file in a format that can be more easily handled by Bash. That would be able to drop the ruby call both in here and formulae.sh which seems likely to be a bunch faster still.

I've been thinking about this too. But I think I picked it up from an idea @Rylan12 mentioned in another PR.

Yeah, I think this was a suggestion about improving autocompletions: basically to also write every formula/cask name to a text file when downloading the API stuff. Then using that text file to easily get formula completions in bash without needing to run brew formulae (and therefore, ruby) for every completion attempt. So yeah, basically the same thing I think you're describing here.

#14259 (comment)

@Bo98
Copy link
Member Author

Bo98 commented Feb 15, 2023

Will look at the txt idea as a second pass as the improvements here are significant enough to not sit on (I'm not sure when 4.0.0 is and I might not be able to revisit this tonight - maybe tomorrow night).

@Bo98 Bo98 merged commit 724e3e6 into Homebrew:master Feb 15, 2023
@Bo98 Bo98 deleted the prefix.sh-api branch February 15, 2023 02:34
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. install from api Relates to API installs outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants