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

Refactor method to remove extra tap requires #18010

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Aug 10, 2024

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

We were selectively requiring the tap.rb file in a few places for performance reasons. The main method we were referencing was the Tap.cmd_directories method which uses Pathname and the TAP_DIRECTORY constant internally. Tap.cmd_directories is mostly used in the Commands module and that is loaded very early on in the program so it made sense to move that command to that module. To facilitate that I moved the TAP_DIRECTORY constant to the top-level and renamed it to HOMEBREW_TAP_DIRECTORY. It now lies in the tap_constants.rb file.

A nice bonus of this refactor is that it speeds up loading external commands since the tap.rb file is no longer required by default in those cases.

$ hyperfine --parameter-list branch master,refactor-method-to-remove-extra-tap-requires --war
mup 5 --setup 'git switch {branch}' 'brew branch-compare -h'
Benchmark 1: brew branch-compare -h (branch = master)
  Time (mean ± σ):     869.5 ms ±   5.1 ms    [User: 506.0 ms, System: 333.7 ms]
  Range (min … max):   858.3 ms … 876.7 ms    10 runs
 
Benchmark 2: brew branch-compare -h (branch = refactor-method-to-remove-extra-tap-requires)
  Time (mean ± σ):     723.3 ms ±   5.7 ms    [User: 418.1 ms, System: 275.5 ms]
  Range (min … max):   713.6 ms … 733.1 ms    10 runs
 
Summary
  brew branch-compare -h (branch = refactor-method-to-remove-extra-tap-requires) ran
    1.20 ± 0.01 times faster than brew branch-compare -h (branch = master)

brew commands also gets a speed boost since Commands.commands no longer requires tap.rb.

$ hyperfine --parameter-list branch master,refactor-method-to-remove-extra-tap-requires --war
mup 5 --setup 'git switch {branch}' 'brew commands'
Benchmark 1: brew commands (branch = master)
  Time (mean ± σ):     875.6 ms ±   8.0 ms    [User: 512.5 ms, System: 332.6 ms]
  Range (min … max):   864.7 ms … 889.5 ms    10 runs
 
Benchmark 2: brew commands (branch = refactor-method-to-remove-extra-tap-requires)
  Time (mean ± σ):     723.4 ms ±   5.5 ms    [User: 420.1 ms, System: 274.2 ms]
  Range (min … max):   718.4 ms … 733.8 ms    10 runs
 
Summary
  brew commands (branch = refactor-method-to-remove-extra-tap-requires) ran
    1.21 ± 0.01 times faster than brew commands (branch = master)

Note: All benchmarks are on an old iMac so may not be as noticeable on newer machines.

Risks: It's possible that another file was counting on the inclusion of tap.rb somewhere else in the codebase and this causes a missing require error. We have no easy way to test for that that I'm aware of.

We were selectively requiring the tap.rb file in a few places for
performance reasons. The main method we were referencing was the
`Tap.cmd_directories` method which uses `Pathname` and the `TAP_DIRECTORY`
constant internally. `Tap.cmd_directories` is mostly used in the `Commands`
module and that is loaded very early on in the program so it made sense
to move that command to that module. To facilitate that I moved the
`TAP_DIRECTORY` constant to the top-level and renamed it to
`HOMEBREW_TAP_DIRECTORY`. It now lies in the tap_constants.rb file.

A nice bonus of this refactor is that it speeds up loading external
commands since the tap.rb file is no longer required by default in
those cases.
Library/Homebrew/brew.rb Outdated Show resolved Hide resolved
@apainintheneck apainintheneck marked this pull request as ready for review August 10, 2024 21:31
- Move HOMEBREW_TAP_DIRECTORY to startup/config.rb because this file
holds more of the directory constants
- Rename `Commands.cmd_directories` to `Commands.tap_cmd_directories`
to better express that the commands come from taps

This file has the directory constants while the other one has regexes.
Just better organization.
@apainintheneck
Copy link
Contributor Author

To recap the main changes here are now:

  • Tap::TAP_DIRECTORY -> HOMEBREW_TAP_DIRECTORY
  • Tap.cmd_directories -> Commands.tap_cmd_directories

I checked that the old names aren't referenced in any of the core taps I have installed locally.

$ pwd
/usr/local/Homebrew/Library/Taps
$ git grep --no-index TAP_DIRECTORY
$ git grep --no-index cmd_directories
$ brew tap
apainintheneck/dev-utils
homebrew/aliases
homebrew/autoupdate
homebrew/bundle
homebrew/cask
homebrew/cask-versions
homebrew/command-not-found
homebrew/core
homebrew/services
homebrew/test-bot
lifepillar/appleii

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.

This is great, wonderful work @apainintheneck 👏🏻.

Given how often these require changes sadly go 💥: let's hold off until after the next tag (which will be later today UK time, merge Tuesday UK time?)

@MikeMcQuaid MikeMcQuaid merged commit e3f8081 into Homebrew:master Aug 12, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants