Skip to content

Commit

Permalink
tap: memoize loading installed taps
Browse files Browse the repository at this point in the history
This adds a new `Tap.tap_registry` method that will load all
installed and core taps only once instead of doing it every time
on the fly. We only expect this list to change when a tap is
installed or uninstalled in the lifetime of the program. The only
places where that can happen are in the `Tap#install` and
`Tap#uninstall` methods so we make sure to update the tap registry
when those methods are called.

This increase the performance of `Tap#reverse_tap_migrations_renames`
since most of that time was spent in `Tap.each` loading the installed
taps from the tap directory.

On my local computer, the performance goes from 3+ seconds to
around 0.2 when calling `Tap.each(&:to_s)` in a loop 6k times.
6k is around the number of formulae in the core tap so this
is not unheard of.

Other changes:
- Override `Tap#eql?` and `Tap.hash` so we can store them uniquely in sets
- Update tests to check that taps are getting added and removed from the
  registry when they get installed and uninstalled
  • Loading branch information
apainintheneck committed Mar 6, 2024
1 parent d55fa09 commit 7961069
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 22 deletions.
2 changes: 1 addition & 1 deletion Library/Homebrew/cmd/tap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def tap
tap.fix_remote_configuration
end
elsif args.no_named?
puts Tap.select(&:installed?)
puts Tap.select(&:installed?).sort_by(&:name)
else
tap = Tap.fetch(args.named.first)
begin
Expand Down
42 changes: 25 additions & 17 deletions Library/Homebrew/tap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
require "completions"
require "extend/cachable"
require "description_cache_store"
require "set"
require "settings"

# A {Tap} is used to extend the formulae provided by Homebrew core.
Expand Down Expand Up @@ -401,6 +402,7 @@ def install(quiet: false, clone_target: nil, force_auto_update: nil,

Commands.rebuild_commands_completion_list
link_completions_and_manpages
Tap.registry << self

formatted_contents = contents.presence&.to_sentence&.dup&.prepend(" ")
$stderr.puts "Tapped#{formatted_contents} (#{path.abv})." unless quiet
Expand Down Expand Up @@ -511,6 +513,7 @@ def uninstall(manual: false)

Commands.rebuild_commands_completion_list
clear_cache
Tap.registry.delete(self)

return if !manual || !official?

Expand Down Expand Up @@ -880,27 +883,32 @@ def ==(other)
other = Tap.fetch(other) if other.is_a?(String)
self.class == other.class && name == other.name
end
alias eql? ==

def self.each(&block)
return to_enum unless block
sig { returns(Integer) }
def hash
[self.class, name].hash
end

installed_taps = if TAP_DIRECTORY.directory?
TAP_DIRECTORY.subdirs
.flat_map(&:subdirs)
.map(&method(:from_path))
else
[]
end
# A set with all installed and core taps.
#
# @private
sig { returns(T::Set[Tap]) }
def self.registry
cache[:registry] ||= Set.new.tap do |set|
if TAP_DIRECTORY.directory?
set.merge TAP_DIRECTORY.subdirs
.flat_map(&:subdirs)
.map(&method(:from_path))
end

available_taps = if Homebrew::EnvConfig.no_install_from_api?
installed_taps
else
default_taps = T.let([CoreTap.instance], T::Array[Tap])
default_taps << CoreCaskTap.instance if OS.mac? # rubocop:disable Homebrew/MoveToExtendOS
installed_taps + default_taps
end.sort_by(&:name).uniq
set << CoreTap.instance
set << CoreCaskTap.instance if OS.mac? # rubocop:disable Homebrew/MoveToExtendOS
end
end

available_taps.each(&block)
def self.each(&block)
registry.each(&block)
end

# An array of all installed {Tap} names.
Expand Down
25 changes: 21 additions & 4 deletions Library/Homebrew/test/tap_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,6 @@ def setup_completion(link:)
end
end

specify "::names" do
expect(described_class.names.sort).to eq(["homebrew/core", "homebrew/foo"])
end

specify "attributes" do
expect(homebrew_foo_tap.user).to eq("Homebrew")
expect(homebrew_foo_tap.repo).to eq("foo")
Expand Down Expand Up @@ -404,17 +400,20 @@ def setup_completion(link:)
setup_completion link: true

tap = described_class.new("Homebrew", "bar")
expect(described_class.registry).not_to include(tap)

tap.install clone_target: homebrew_foo_tap.path/".git"

expect(tap).to be_installed
expect(described_class.registry).to include(tap)
expect(HOMEBREW_PREFIX/"share/man/man1/brew-tap-cmd.1").to be_a_file
expect(HOMEBREW_PREFIX/"etc/bash_completion.d/brew-tap-cmd").to be_a_file
expect(HOMEBREW_PREFIX/"share/zsh/site-functions/_brew-tap-cmd").to be_a_file
expect(HOMEBREW_PREFIX/"share/fish/vendor_completions.d/brew-tap-cmd.fish").to be_a_file
tap.uninstall

expect(tap).not_to be_installed
expect(described_class.registry).not_to include(tap)
expect(HOMEBREW_PREFIX/"share/man/man1/brew-tap-cmd.1").not_to exist
expect(HOMEBREW_PREFIX/"share/man/man1").not_to exist
expect(HOMEBREW_PREFIX/"etc/bash_completion.d/brew-tap-cmd").not_to exist
Expand Down Expand Up @@ -501,6 +500,24 @@ def setup_completion(link:)
end
end

describe ".registry" do
it "includes the core and cask taps by default", :needs_macos do
expect(described_class.registry).to include(CoreTap.instance, CoreCaskTap.instance)
end

it "includes the core tap and excludes the cask tap by default", :needs_linux do
expect(described_class.registry).to include(CoreTap.instance)
expect(described_class.registry).not_to include(CoreCaskTap.instance)
end

it "ignores a second instance of the same tap" do
expect { described_class.registry << described_class.new("example", "first") }
.to change { described_class.registry.size }.by(1)
expect { described_class.registry << described_class.new("example", "first") }
.not_to(change { described_class.registry.size })
end
end

describe "Formula Lists" do
describe "#formula_renames" do
it "returns the formula_renames hash" do
Expand Down

0 comments on commit 7961069

Please sign in to comment.