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.

Sign up for GitHub

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

Jump to bottom

Enable HOMEBREW_BOOTSNAP for developers #10680

Merged
merged 1 commit into from Feb 24, 2021
Merged

Enable HOMEBREW_BOOTSNAP for developers #10680

merged 1 commit into from Feb 24, 2021

Conversation

MikeMcQuaid
Copy link
Member

  • Enable it by default if you've run a developer command or set HOMEBREW_DEVELOPER.
  • Clarify the documentation that there's various configuration in which it doesn't work.

I've rolled this out of most of GitHub's developers with no ill effects and we've enabled it in both CI and for various maintainers so it feels appropriate to have a bigger rollout. @Homebrew/brew: any thoughts?

@MikeMcQuaid
Enable HOMEBREW_BOOTSNAP for developers
22fb25d 
- Enable it by default if you've run a developer command or set
  `HOMEBREW_DEVELOPER`.
- Clarify the documentation that there's various configuration in which
  it doesn't work.

I've rolled this out of most of GitHub's developers with no ill effects
and we've enabled it in both CI and for various maintainers so it feels
appropriate to have a bigger rollout.
@BrewTestBot
Copy link
Member

Review period will end on 2021-02-24 at 09:35:27 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 23, 2021
Rylan12
Rylan12 approved these changes Feb 23, 2021
View reviewed changes
Copy link
Member

@Rylan12 Rylan12 left a comment •
edited

Choose a reason for hiding this comment

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

I, personally, haven't been using this, but I'm 👍 to start enabling it for developers (and myself) based on the success so far (which I can conclude based on the lack of PRs or issues about fixing it)

@fxcoudert
Copy link
Member

I've just experimented with it:

  • it doesn't speed up the most common operation for mer, which is brew up (360 ms on my system, with or without HOMEBREW_BOOTSNAP, when I'm already up-to-date)
  • brew config goes down from 0.8 s to 0.6 s
  • brew ls goes down from 0.55 s to 0.35 s
  • brew cleanup (with nothing to clean up) goes up from 1.0 s to 1.2 s

fxcoudert
fxcoudert reviewed Feb 23, 2021
View reviewed changes
Library/Homebrew/brew.sh
@@ -584,6 +584,11 @@ then
export HOMEBREW_BOTTLE_DOMAIN="$HOMEBREW_BOTTLE_DEFAULT_DOMAIN"
fi

if [[ -n "$HOMEBREW_DEVELOPER" || -n "$HOMEBREW_DEV_CMD_RUN" ]]
then
export HOMEBREW_BOOTSNAP="1"
Copy link
Member

Choose a reason for hiding this comment

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

You would have better performance if you rewrote brew in rust.

(Don't hit me, I'm just kidding!)

Copy link
Member

Choose a reason for hiding this comment

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

Wait, HOMEBREW_BOOTSNAP doesn't run Homebrew in Rust? That's what I thought it did...

Copy link
Member

Choose a reason for hiding this comment

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

I look forward to the day where we install brew to build brew.

@MikeMcQuaid
Copy link
Member Author

  • it doesn't speed up the most common operation for mer, which is brew up (360 ms on my system, with or without HOMEBREW_BOOTSNAP, when I'm already up-to-date)
  • brew cleanup (with nothing to clean up) goes up from 1.0 s to 1.2 s

These are IO bound and brew cleanup removes the existing bootsnap cache.

  • brew config goes down from 0.8 s to 0.6 s
  • brew ls goes down from 0.55 s to 0.35 s

The speedup here is mostly due to speeding up require pretty dramatically.

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

Review period ended.

@MikeMcQuaid MikeMcQuaid merged commit 3a73a7b into Homebrew:master Feb 24, 2021
@MikeMcQuaid MikeMcQuaid deleted the bootsnap-developers branch February 24, 2021 13:46
@Bo98
Copy link
Member

Bo98 commented Feb 24, 2021

Just as a FYI: Homebrew/discussions#923

(I think it might be fixable with some SDKROOT stuff, but I need more information on their setups first.)

@Bo98
Copy link
Member

Bo98 commented Feb 24, 2021 •
edited

Actually maybe we should have a DevelopmentTools.installed? check here. I suspect running without them will cause issues, since native extensions need compiling.

@MikeMcQuaid
Copy link
Member Author

@Bo98 #10694

@MikeMcQuaid MikeMcQuaid mentioned this pull request Feb 25, 2021
Merged
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 27, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Bo98 Bo98 Bo98 left review comments

@fxcoudert fxcoudert fxcoudert left review comments

@carlocab carlocab carlocab left review comments

@BrewTestBot BrewTestBot BrewTestBot approved these changes

@Rylan12 Rylan12 Rylan12 approved these changes

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MikeMcQuaid @BrewTestBot @fxcoudert @Bo98 @carlocab @Rylan12