From 752a1d785475d1950b33c7d77b289a6a3a753f02 Mon Sep 17 00:00:00 2001 From: Hartley McGuire Date: Tue, 25 Mar 2025 23:27:09 -0400 Subject: [PATCH] [rubygems/rubygems] Implement pub_grub strategy interface My application spends more than 30% of time during `bundle update` comparing versions due to versions being sorted inside next_package_to_try. This has been addressed in pub_grub by defining a strategy interface (a `#next_package_and_version` method) which allows consumers to have finer control over the heuristic to select the next package to try. This commit implements the new strategy interface to remove extraneous version sorting (previously in next_package_to_try) since only the final count of versions is used. Combined with a previous change to pub_grub (already applied to Bundler), this commit results in `bundle update` taking only half the time it did on 2.6.5. https://github.com/rubygems/rubygems/commit/62f69e27f0 --- lib/bundler/resolver.rb | 29 ++++--------- lib/bundler/resolver/strategy.rb | 40 ++++++++++++++++++ .../lib/pub_grub/basic_package_source.rb | 26 ++---------- .../vendor/pub_grub/lib/pub_grub/strategy.rb | 42 +++++++++++++++++++ .../pub_grub/lib/pub_grub/version_solver.rb | 23 ++++------ tool/bundler/vendor_gems.rb | 2 +- tool/bundler/vendor_gems.rb.lock | 4 +- 7 files changed, 105 insertions(+), 61 deletions(-) create mode 100644 lib/bundler/resolver/strategy.rb create mode 100644 lib/bundler/vendor/pub_grub/lib/pub_grub/strategy.rb diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb index c25076dc1d..5fb826380c 100644 --- a/lib/bundler/resolver.rb +++ b/lib/bundler/resolver.rb @@ -12,6 +12,7 @@ module Bundler require_relative "resolver/candidate" require_relative "resolver/incompatibility" require_relative "resolver/root" + require_relative "resolver/strategy" include GemHelpers @@ -78,7 +79,7 @@ module Bundler end def solve_versions(root:, logger:) - solver = PubGrub::VersionSolver.new(source: self, root: root, logger: logger) + solver = PubGrub::VersionSolver.new(source: self, root: root, strategy: Strategy.new(self), logger: logger) result = solver.solve resolved_specs = result.flat_map {|package, version| version.to_specs(package, @most_specific_locked_platform) } SpecSet.new(resolved_specs).specs_with_additional_variants_from(@base.locked_specs) @@ -167,15 +168,7 @@ module Bundler end def versions_for(package, range=VersionRange.any) - versions = select_sorted_versions(package, range) - - # Conditional avoids (among other things) calling - # sort_versions_by_preferred with the root package - if versions.size > 1 - sort_versions_by_preferred(package, versions) - else - versions - end + range.select_versions(@sorted_versions[package]) end def no_versions_incompatibility_for(package, unsatisfied_term) @@ -355,6 +348,10 @@ module Bundler raise GemNotFound, message end + def sort_versions_by_preferred(package, versions) + @gem_version_promoter.sort_versions(package, versions) + end + private def filtered_versions_for(package) @@ -414,10 +411,6 @@ module Bundler requirement.satisfied_by?(spec.version) || spec.source.is_a?(Source::Gemspec) end - def sort_versions_by_preferred(package, versions) - @gem_version_promoter.sort_versions(package, versions) - end - def repository_for(package) source_for(package.name) end @@ -433,7 +426,7 @@ module Bundler next [dep_package, dep_constraint] if name == "bundler" dep_range = dep_constraint.range - versions = select_sorted_versions(dep_package, dep_range) + versions = versions_for(dep_package, dep_range) if versions.empty? if dep_package.ignores_prereleases? || dep_package.prefer_local? @all_versions.delete(dep_package) @@ -441,7 +434,7 @@ module Bundler end dep_package.consider_prereleases! if dep_package.ignores_prereleases? dep_package.consider_remote_versions! if dep_package.prefer_local? - versions = select_sorted_versions(dep_package, dep_range) + versions = versions_for(dep_package, dep_range) end if versions.empty? && select_all_versions(dep_package, dep_range).any? @@ -456,10 +449,6 @@ module Bundler end.to_h end - def select_sorted_versions(package, range) - range.select_versions(@sorted_versions[package]) - end - def select_all_versions(package, range) range.select_versions(@all_versions[package]) end diff --git a/lib/bundler/resolver/strategy.rb b/lib/bundler/resolver/strategy.rb new file mode 100644 index 0000000000..4f343bf0ce --- /dev/null +++ b/lib/bundler/resolver/strategy.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module Bundler + class Resolver + class Strategy + def initialize(source) + @source = source + end + + def next_package_and_version(unsatisfied) + package, range = next_term_to_try_from(unsatisfied) + + [package, most_preferred_version_of(package, range).first] + end + + private + + def next_term_to_try_from(unsatisfied) + unsatisfied.min_by do |package, range| + matching_versions = @source.versions_for(package, range) + higher_versions = @source.versions_for(package, range.upper_invert) + + [matching_versions.count <= 1 ? 0 : 1, higher_versions.count] + end + end + + def most_preferred_version_of(package, range) + versions = @source.versions_for(package, range) + + # Conditional avoids (among other things) calling + # sort_versions_by_preferred with the root package + if versions.size > 1 + @source.sort_versions_by_preferred(package, versions) + else + versions + end + end + end + end +end diff --git a/lib/bundler/vendor/pub_grub/lib/pub_grub/basic_package_source.rb b/lib/bundler/vendor/pub_grub/lib/pub_grub/basic_package_source.rb index 27f0e39d4c..491151ec0b 100644 --- a/lib/bundler/vendor/pub_grub/lib/pub_grub/basic_package_source.rb +++ b/lib/bundler/vendor/pub_grub/lib/pub_grub/basic_package_source.rb @@ -79,29 +79,17 @@ module Bundler::PubGrub dependencies_for(@root_package, @root_version) end - # Override me (maybe) - # - # If not overridden, the order returned by all_versions_for will be used - # - # Returns: Array of versions in preferred order - def sort_versions_by_preferred(package, sorted_versions) - indexes = @version_indexes[package] - sorted_versions.sort_by { |version| indexes[version] } - end - def initialize @root_package = Package.root @root_version = Package.root_version - @cached_versions = Hash.new do |h,k| + @sorted_versions = Hash.new do |h,k| if k == @root_package h[k] = [@root_version] else - h[k] = all_versions_for(k) + h[k] = all_versions_for(k).sort end end - @sorted_versions = Hash.new { |h,k| h[k] = @cached_versions[k].sort } - @version_indexes = Hash.new { |h,k| h[k] = @cached_versions[k].each.with_index.to_h } @cached_dependencies = Hash.new do |packages, package| if package == @root_package @@ -117,15 +105,7 @@ module Bundler::PubGrub end def versions_for(package, range=VersionRange.any) - versions = range.select_versions(@sorted_versions[package]) - - # Conditional avoids (among other things) calling - # sort_versions_by_preferred with the root package - if versions.size > 1 - sort_versions_by_preferred(package, versions) - else - versions - end + range.select_versions(@sorted_versions[package]) end def no_versions_incompatibility_for(_package, unsatisfied_term) diff --git a/lib/bundler/vendor/pub_grub/lib/pub_grub/strategy.rb b/lib/bundler/vendor/pub_grub/lib/pub_grub/strategy.rb new file mode 100644 index 0000000000..6955655ba4 --- /dev/null +++ b/lib/bundler/vendor/pub_grub/lib/pub_grub/strategy.rb @@ -0,0 +1,42 @@ +module Bundler::PubGrub + class Strategy + def initialize(source) + @source = source + + @root_package = Package.root + @root_version = Package.root_version + + @version_indexes = Hash.new do |h,k| + if k == @root_package + h[k] = { @root_version => 0 } + else + h[k] = @source.all_versions_for(k).each.with_index.to_h + end + end + end + + def next_package_and_version(unsatisfied) + package, range = next_term_to_try_from(unsatisfied) + + [package, most_preferred_version_of(package, range)] + end + + private + + def most_preferred_version_of(package, range) + versions = @source.versions_for(package, range) + + indexes = @version_indexes[package] + versions.min_by { |version| indexes[version] } + end + + def next_term_to_try_from(unsatisfied) + unsatisfied.min_by do |package, range| + matching_versions = @source.versions_for(package, range) + higher_versions = @source.versions_for(package, range.upper_invert) + + [matching_versions.count <= 1 ? 0 : 1, higher_versions.count] + end + end + end +end diff --git a/lib/bundler/vendor/pub_grub/lib/pub_grub/version_solver.rb b/lib/bundler/vendor/pub_grub/lib/pub_grub/version_solver.rb index b56b8ce43e..000923e99a 100644 --- a/lib/bundler/vendor/pub_grub/lib/pub_grub/version_solver.rb +++ b/lib/bundler/vendor/pub_grub/lib/pub_grub/version_solver.rb @@ -2,17 +2,20 @@ require_relative 'partial_solution' require_relative 'term' require_relative 'incompatibility' require_relative 'solve_failure' +require_relative 'strategy' module Bundler::PubGrub class VersionSolver attr_reader :logger attr_reader :source attr_reader :solution + attr_reader :strategy - def initialize(source:, root: Package.root, logger: Bundler::PubGrub.logger) + def initialize(source:, root: Package.root, strategy: Strategy.new(source), logger: Bundler::PubGrub.logger) @logger = logger @source = source + @strategy = strategy # { package => [incompatibility, ...]} @incompatibilities = Hash.new do |h, k| @@ -104,25 +107,15 @@ module Bundler::PubGrub unsatisfied.package end - def next_term_to_try_from(unsatisfied_terms) - unsatisfied_terms.min_by do |term| - package = term.package - range = term.constraint.range - matching_versions = source.versions_for(package, range) - higher_versions = source.versions_for(package, range.upper_invert) - - [matching_versions.count <= 1 ? 0 : 1, higher_versions.count] - end - end - def choose_package_version_from(unsatisfied_terms) - unsatisfied_term = next_term_to_try_from(unsatisfied_terms) - package = unsatisfied_term.package + remaining = unsatisfied_terms.map { |t| [t.package, t.constraint.range] }.to_h + + package, version = strategy.next_package_and_version(remaining) - version = source.versions_for(package, unsatisfied_term.constraint.range).first logger.debug { "attempting #{package} #{version}" } if version.nil? + unsatisfied_term = unsatisfied_terms.find { |t| t.package == package } add_incompatibility source.no_versions_incompatibility_for(package, unsatisfied_term) return package end diff --git a/tool/bundler/vendor_gems.rb b/tool/bundler/vendor_gems.rb index 16fe53a5d8..467bbf33b3 100644 --- a/tool/bundler/vendor_gems.rb +++ b/tool/bundler/vendor_gems.rb @@ -8,7 +8,7 @@ gem "net-http", "0.6.0" gem "net-http-persistent", "4.0.4" gem "net-protocol", "0.2.2" gem "optparse", "0.6.0" -gem "pub_grub", github: "jhawthorn/pub_grub", ref: "57d4f344366c8b86f7fe506e9bfa08f3c731e397" +gem "pub_grub", github: "jhawthorn/pub_grub", ref: "df6add45d1b4d122daff2f959c9bd1ca93d14261" gem "resolv", "0.6.0" gem "securerandom", "0.4.1" gem "timeout", "0.4.3" diff --git a/tool/bundler/vendor_gems.rb.lock b/tool/bundler/vendor_gems.rb.lock index 79eca0e7c0..55ac15bd03 100644 --- a/tool/bundler/vendor_gems.rb.lock +++ b/tool/bundler/vendor_gems.rb.lock @@ -6,8 +6,8 @@ GIT GIT remote: https://github.com/jhawthorn/pub_grub.git - revision: 57d4f344366c8b86f7fe506e9bfa08f3c731e397 - ref: 57d4f344366c8b86f7fe506e9bfa08f3c731e397 + revision: df6add45d1b4d122daff2f959c9bd1ca93d14261 + ref: df6add45d1b4d122daff2f959c9bd1ca93d14261 specs: pub_grub (0.5.0)