From 7961069f697a5526f1001a5f19ce13f2a345750d Mon Sep 17 00:00:00 2001 From: apainintheneck Date: Sun, 3 Mar 2024 12:29:43 -0800 Subject: [PATCH] tap: memoize loading installed taps 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 --- Library/Homebrew/cmd/tap.rb | 2 +- Library/Homebrew/tap.rb | 42 ++++++++++++++++++------------- Library/Homebrew/test/tap_spec.rb | 25 +++++++++++++++--- 3 files changed, 47 insertions(+), 22 deletions(-) diff --git a/Library/Homebrew/cmd/tap.rb b/Library/Homebrew/cmd/tap.rb index f3f751ead323b..88953928fb15a 100644 --- a/Library/Homebrew/cmd/tap.rb +++ b/Library/Homebrew/cmd/tap.rb @@ -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 diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index c22a1325a4cd9..06f4dde26e3cf 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -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. @@ -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 @@ -511,6 +513,7 @@ def uninstall(manual: false) Commands.rebuild_commands_completion_list clear_cache + Tap.registry.delete(self) return if !manual || !official? @@ -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. diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb index cc7b7f5edaa1f..0b48c3c6abc8c 100644 --- a/Library/Homebrew/test/tap_spec.rb +++ b/Library/Homebrew/test/tap_spec.rb @@ -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") @@ -404,10 +400,12 @@ 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 @@ -415,6 +413,7 @@ def setup_completion(link:) 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 @@ -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