Skip to content

Lazily require some heavy files#8382

Merged
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
Bo98:startup-speedup
Aug 21, 2020
Merged

Lazily require some heavy files#8382
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
Bo98:startup-speedup

Conversation

@Bo98
Copy link
Copy Markdown
Member

@Bo98 Bo98 commented Aug 17, 2020 •
edited
Loading

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The changes here are to avoid require "formula" on a basic startup path (e.g. brew --version). This saves as much as 150ms in boot time.

Parts of brew already did this, but it slowly stopped being the case over the years.

Notably:

  • Moved the requires in the cask commands to make them only happen when the command is executed. Cask commands are not lazily loaded like other brew commands and some of the requires like cask/installer are among the heaviest in Homebrew.
  • formula is now required inside Formulary's load_formula method. This covers the vast majority of cases were require "formula" was needed.
  • readall is now lazily loaded in Tap#install as it has a require "formula", like descriptions already is for the same reason.

@Bo98 Bo98 force-pushed the startup-speedup branch 4 times, most recently from 323ea5f to e649cf5 Compare August 18, 2020 00:23
@Bo98 Bo98 mentioned this pull request Aug 18, 2020
Merged
MikeMcQuaid
MikeMcQuaid approved these changes Aug 18, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Nice if this speeds things up. My only thought/concern: how do we stop this happening again?

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Aug 18, 2020

A test that makes sure Formula is undefined on a basic startup would work, though I don’t entirely know yet how to do that.

@MikeMcQuaid
Copy link
Copy Markdown
Member

MikeMcQuaid commented Aug 18, 2020

A test that makes sure Formula is undefined on a basic startup would work, though I don’t entirely know yet how to do that.

Makes sense 👍🏻.

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Aug 18, 2020

Minor point that might not matter much:

Is losing Formula[] unless require "formula" is done a concern at all? Seemingly not as I've not really seen much break from it - but I just wanted to raise that as moving that specifically to formulary.rb (or whatever) is possible, which would mean that Formula["whatever"] could automatically invoke require "formula" like Formulary.factory("whatever") does.

@MikeMcQuaid
Copy link
Copy Markdown
Member

MikeMcQuaid commented Aug 18, 2020

Is losing Formula[] unless require "formula" is done a concern at all?

I don't think so.

which would mean that Formula["whatever"] could automatically invoke require "formula" like Formulary.factory("whatever") does.

I think it's nicer for Formula methods to continue to live in formula.rb.

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Aug 18, 2020 •
edited
Loading

I don't think so.

Fine with me! Just wanted to make sure someone else agreed.

@Bo98 Bo98 force-pushed the startup-speedup branch 3 times, most recently from fc0ecf7 to 8d51bec Compare August 18, 2020 15:57
@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Aug 18, 2020 •
edited
Loading

It is using the v1-style cmd system (though with Homebrew::CLI parsing), but I did get get a test working to check for this.

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Aug 18, 2020

Remaining error is homebrew-services not being updated properly in the CI - I'll have a look.

@Bo98
Copy link
Copy Markdown
Member Author

Bo98 commented Aug 18, 2020

#8391

@Bo98 Bo98 force-pushed the startup-speedup branch from 8d51bec to 3a2fad0 Compare August 19, 2020 15:15
@Bo98 Bo98 force-pushed the startup-speedup branch from 3a2fad0 to 281632b Compare August 20, 2020 01:05
@MikeMcQuaid MikeMcQuaid merged commit 667c2b8 into Homebrew:master Aug 21, 2020
@MikeMcQuaid
Copy link
Copy Markdown
Member

MikeMcQuaid commented Aug 21, 2020

Thanks @Bo98!

dawidd6 added a commit to dawidd6/homebrew-livecheck that referenced this pull request Aug 21, 2020
@dawidd6
cmd/livecheck: require "formula"
523a8e1 
Fix:
```
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/dawidd6/homebrew-test/Formula/test-formula-url.rb:2:in `<class:TestFormulaUrl>'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/dawidd6/homebrew-test/Formula/test-formula-url.rb:1:in `load_formula'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-livecheck/livecheck/extend/formulary.rb:22:in `module_eval'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-livecheck/livecheck/extend/formulary.rb:22:in `load_formula'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formulary.rb:64:in `load_formula_from_path'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formulary.rb:149:in `load_file'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formulary.rb:294:in `load_file'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formulary.rb:139:in `klass'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formulary.rb:135:in `get_formula'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formulary.rb:284:in `get_formula'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formulary.rb:344:in `factory'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/cli/named_args.rb:21:in `block in to_formulae'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/cli/named_args.rb:20:in `map'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/cli/named_args.rb:20:in `to_formulae'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/cli/args.rb:60:in `formulae'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-livecheck/cmd/livecheck.rb:94:in `livecheck'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb:119:in `<main>'
Error: dawidd6/test/test-formula-url: undefined method `desc' for #<Class:0x000000000328e300>
```

After latest changes in `brew`:
Homebrew/brew#8382
@miccal
Copy link
Copy Markdown
Contributor

miccal commented Aug 21, 2020 •
edited
Loading

Using git bisect, I believe the merging of this PR has cause an issue in homebrew-cask -- for example, in Homebrew/homebrew-cask#87951:

Error: Cask 'cutter' definition is invalid: invalid 'depends_on macos' value: ">= :sierra"

and this error occurs for any cask with a depends_on macos: when brew cask audit or brew cask style is run:

-> brew cask audit cutter
Error: Cask 'cutter' definition is invalid: invalid 'depends_on macos' value: ">= :sierra"

|-> brew cask style cutter
Error: Cask 'cutter' definition is invalid: invalid 'depends_on macos' value: ">= :sierra"

@miccal miccal mentioned this pull request Aug 21, 2020
Merged
@Bo98 Bo98 mentioned this pull request Aug 21, 2020
Merged
6 tasks
@miccal
Copy link
Copy Markdown
Contributor

miccal commented Aug 21, 2020 •
edited
Loading

Thank you @Bo98, you were much faster than I was in spotting the problem!

@Bo98 Bo98 deleted the startup-speedup branch August 21, 2020 17:42
@miccal miccal mentioned this pull request Aug 21, 2020
Closed
6 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 16, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid 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.

4 participants

@Bo98 @MikeMcQuaid @miccal @BrewTestBot