From d47eb8bf57c32c981ac44cb06bfe4c22bb24039e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Tue, 17 May 2022 10:37:05 +0200 Subject: [PATCH 1/7] Deduplication is only necessary for materialization --- bundler/lib/bundler/spec_set.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/bundler/lib/bundler/spec_set.rb b/bundler/lib/bundler/spec_set.rb index 0dfaed980771..cc10104a3654 100644 --- a/bundler/lib/bundler/spec_set.rb +++ b/bundler/lib/bundler/spec_set.rb @@ -40,8 +40,6 @@ def for(dependencies, check = false, match_current_platform = false) specs << spec end - specs.uniq! unless match_current_platform - check ? true : specs end @@ -69,7 +67,7 @@ def to_hash end def materialize(deps) - materialized = self.for(deps, false, true) + materialized = self.for(deps, false, true).uniq materialized.map! do |s| next s unless s.is_a?(LazySpecification) From 00e6baaefbb5eeab23202d99fa53103f8ad7e810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Fri, 29 Apr 2022 13:18:50 +0200 Subject: [PATCH 2/7] Refactor `SpecSet#for` So that it deals with [name, platform] tuples consistently instead of dealing with `Gem::Dependency` or `Bundler::DepProxy` instances inconsistently. --- bundler/lib/bundler/definition.rb | 8 ++++---- bundler/lib/bundler/resolver.rb | 2 +- bundler/lib/bundler/spec_set.rb | 23 +++++++++++------------ 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/bundler/lib/bundler/definition.rb b/bundler/lib/bundler/definition.rb index 4fca763bcce5..9f0e54fd96fb 100644 --- a/bundler/lib/bundler/definition.rb +++ b/bundler/lib/bundler/definition.rb @@ -138,13 +138,13 @@ def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, opti @unlock[:gems] ||= @dependencies.map(&:name) else eager_unlock = expand_dependencies(@unlock[:gems] || [], true) - @unlock[:gems] = @locked_specs.for(eager_unlock, false, false).map(&:name) + @unlock[:gems] = @locked_specs.for(eager_unlock, false, platforms).map(&:name) end @dependency_changes = converge_dependencies @local_changes = converge_locals - @locked_specs_incomplete_for_platform = !@locked_specs.for(requested_dependencies & expand_dependencies(locked_dependencies), true, true) + @locked_specs_incomplete_for_platform = !@locked_specs.for(requested_dependencies & expand_dependencies(locked_dependencies), true) @requires = compute_requires end @@ -466,7 +466,7 @@ def unlocking? private def filter_specs(specs, deps) - SpecSet.new(specs).for(expand_dependencies(deps, true), false, false) + SpecSet.new(specs).for(expand_dependencies(deps, true), false, platforms) end def materialize(dependencies) @@ -709,7 +709,7 @@ def converge_specs(specs) # if we won't need the source (according to the lockfile), # don't error if the path/git source isn't available next if specs. - for(requested_dependencies, false, true). + for(requested_dependencies, false). none? {|locked_spec| locked_spec.source == s.source } raise diff --git a/bundler/lib/bundler/resolver.rb b/bundler/lib/bundler/resolver.rb index 2285114c5708..db0312488c8d 100644 --- a/bundler/lib/bundler/resolver.rb +++ b/bundler/lib/bundler/resolver.rb @@ -21,7 +21,7 @@ def self.resolve(requirements, source_requirements = {}, base = [], gem_version_ base = SpecSet.new(base) unless base.is_a?(SpecSet) resolver = new(source_requirements, base, gem_version_promoter, additional_base_requirements, platforms) result = resolver.start(requirements) - SpecSet.new(SpecSet.new(result).for(requirements.reject {|dep| dep.name.end_with?("\0") })) + SpecSet.new(SpecSet.new(result).for(requirements.reject {|dep| dep.name.end_with?("\0") }, false, platforms)) end def initialize(source_requirements, base, gem_version_promoter, additional_base_requirements, platforms) diff --git a/bundler/lib/bundler/spec_set.rb b/bundler/lib/bundler/spec_set.rb index cc10104a3654..4b99351f26ce 100644 --- a/bundler/lib/bundler/spec_set.rb +++ b/bundler/lib/bundler/spec_set.rb @@ -11,25 +11,24 @@ def initialize(specs) @specs = specs end - def for(dependencies, check = false, match_current_platform = false) - handled = [] - deps = dependencies.dup + def for(dependencies, check = false, platforms = [nil]) + handled = ["bundler"].product(platforms) + deps = dependencies.map(&:name).product(platforms) specs = [] loop do break unless dep = deps.shift - next if handled.any? {|d| d.name == dep.name && (match_current_platform || d.__platform == dep.__platform) } || dep.name == "bundler" + next if handled.include?(dep) handled << dep - specs_for_dep = spec_for_dependency(dep, match_current_platform) + specs_for_dep = spec_for_dependency(*dep) if specs_for_dep.any? specs.concat(specs_for_dep) specs_for_dep.first.dependencies.each do |d| next if d.type == :development - d = DepProxy.get_proxy(d, dep.__platform) unless match_current_platform - deps << d + deps << [d.name, dep[1]] end elsif check return false @@ -67,7 +66,7 @@ def to_hash end def materialize(deps) - materialized = self.for(deps, false, true).uniq + materialized = self.for(deps, false).uniq materialized.map! do |s| next s unless s.is_a?(LazySpecification) @@ -169,12 +168,12 @@ def tsort_each_node @specs.sort_by(&:name).each {|s| yield s } end - def spec_for_dependency(dep, match_current_platform) - specs_for_platforms = lookup[dep.name] - if match_current_platform + def spec_for_dependency(name, platform) + specs_for_platforms = lookup[name] + if platform.nil? GemHelpers.select_best_platform_match(specs_for_platforms.select {|s| Gem::Platform.match_spec?(s) }, Bundler.local_platform) else - GemHelpers.select_best_platform_match(specs_for_platforms, dep.__platform) + GemHelpers.select_best_platform_match(specs_for_platforms, platform) end end From 3d4f1ceb2a2f5c3ab1c52ce2d5b7955dc7319cf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Tue, 17 May 2022 13:50:53 +0200 Subject: [PATCH 3/7] Remove comment that does not really hold The resolve might be against locally available gems, or remote gems, depending on the situation. --- bundler/lib/bundler/definition.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/bundler/lib/bundler/definition.rb b/bundler/lib/bundler/definition.rb index 9f0e54fd96fb..1d650b192efa 100644 --- a/bundler/lib/bundler/definition.rb +++ b/bundler/lib/bundler/definition.rb @@ -264,7 +264,6 @@ def resolve SpecSet.new(filter_specs(@locked_specs, @dependencies.select {|dep| @locked_specs[dep].any? })) else last_resolve = converge_locked_specs - # Run a resolve against the locally available gems Bundler.ui.debug("Found changes from the lockfile, re-resolving dependencies because #{change_reason}") expanded_dependencies = expand_dependencies(dependencies + metadata_dependencies, true) Resolver.resolve(expanded_dependencies, source_requirements, last_resolve, gem_version_promoter, additional_base_requirements_for_resolve, platforms) From 76d8fec15f1f9a86d67dd0b133236596ae56478a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Mon, 16 May 2022 18:43:56 +0200 Subject: [PATCH 4/7] Delay `SpecSet#for` for checking incomplete platform specs This is a very weird edge case, not all Bundler invokations should pay its cost. Instead, assume it's not going to happen, detect the situation at materialization time, and re-resolve. --- Manifest.txt | 1 + bundler/lib/bundler.rb | 1 + bundler/lib/bundler/definition.rb | 26 ++++++++++++++----- .../lib/bundler/incomplete_specification.rb | 12 +++++++++ bundler/lib/bundler/spec_set.rb | 10 ++++--- 5 files changed, 41 insertions(+), 9 deletions(-) create mode 100644 bundler/lib/bundler/incomplete_specification.rb diff --git a/Manifest.txt b/Manifest.txt index f32bd361afa0..68a545439680 100644 --- a/Manifest.txt +++ b/Manifest.txt @@ -80,6 +80,7 @@ bundler/lib/bundler/gem_helpers.rb bundler/lib/bundler/gem_tasks.rb bundler/lib/bundler/gem_version_promoter.rb bundler/lib/bundler/graph.rb +bundler/lib/bundler/incomplete_specification.rb bundler/lib/bundler/index.rb bundler/lib/bundler/injector.rb bundler/lib/bundler/inline.rb diff --git a/bundler/lib/bundler.rb b/bundler/lib/bundler.rb index 0be01d18088e..6f5b605988ee 100644 --- a/bundler/lib/bundler.rb +++ b/bundler/lib/bundler.rb @@ -53,6 +53,7 @@ module Bundler autoload :GemHelpers, File.expand_path("bundler/gem_helpers", __dir__) autoload :GemVersionPromoter, File.expand_path("bundler/gem_version_promoter", __dir__) autoload :Graph, File.expand_path("bundler/graph", __dir__) + autoload :IncompleteSpecification, File.expand_path("bundler/incomplete_specification", __dir__) autoload :Index, File.expand_path("bundler/index", __dir__) autoload :Injector, File.expand_path("bundler/injector", __dir__) autoload :Installer, File.expand_path("bundler/installer", __dir__) diff --git a/bundler/lib/bundler/definition.rb b/bundler/lib/bundler/definition.rb index 1d650b192efa..ea65cbd06222 100644 --- a/bundler/lib/bundler/definition.rb +++ b/bundler/lib/bundler/definition.rb @@ -144,7 +144,7 @@ def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, opti @dependency_changes = converge_dependencies @local_changes = converge_locals - @locked_specs_incomplete_for_platform = !@locked_specs.for(requested_dependencies & expand_dependencies(locked_dependencies), true) + @reresolve = nil @requires = compute_requires end @@ -263,10 +263,8 @@ def resolve Bundler.ui.debug("Found no changes, using resolution from the lockfile") SpecSet.new(filter_specs(@locked_specs, @dependencies.select {|dep| @locked_specs[dep].any? })) else - last_resolve = converge_locked_specs Bundler.ui.debug("Found changes from the lockfile, re-resolving dependencies because #{change_reason}") - expanded_dependencies = expand_dependencies(dependencies + metadata_dependencies, true) - Resolver.resolve(expanded_dependencies, source_requirements, last_resolve, gem_version_promoter, additional_base_requirements_for_resolve, platforms) + @reresolve = reresolve end end end @@ -455,7 +453,7 @@ def most_specific_locked_platform private :sources def nothing_changed? - !@source_changes && !@dependency_changes && !@new_platform && !@path_changes && !@local_changes && !@locked_specs_incomplete_for_platform + !@source_changes && !@dependency_changes && !@new_platform && !@path_changes && !@local_changes end def unlocking? @@ -464,6 +462,12 @@ def unlocking? private + def reresolve + last_resolve = converge_locked_specs + expanded_dependencies = expand_dependencies(dependencies + metadata_dependencies, true) + Resolver.resolve(expanded_dependencies, source_requirements, last_resolve, gem_version_promoter, additional_base_requirements_for_resolve, platforms) + end + def filter_specs(specs, deps) SpecSet.new(specs).for(expand_dependencies(deps, true), false, platforms) end @@ -485,6 +489,17 @@ def materialize(dependencies) raise GemNotFound, "Could not find #{missing_specs.map(&:full_name).join(", ")} in any of the sources" end + if @reresolve.nil? + incomplete_specs = specs.incomplete_specs + + if incomplete_specs.any? + Bundler.ui.debug("The lockfile does not have all gems needed for the current platform though, Bundler will still re-resolve dependencies") + @unlock[:gems].concat(incomplete_specs.map(&:name)) + @resolve = reresolve + specs = resolve.materialize(dependencies) + end + end + unless specs["bundler"].any? bundler = sources.metadata_source.specs.search(Gem::Dependency.new("bundler", VERSION)).last specs["bundler"] = bundler @@ -532,7 +547,6 @@ def change_reason [@new_platform, "you added a new platform to your gemfile"], [@path_changes, "the gemspecs for path gems changed"], [@local_changes, "the gemspecs for git local gems changed"], - [@locked_specs_incomplete_for_platform, "the lockfile does not have all gems needed for the current platform"], ].select(&:first).map(&:last).join(", ") end diff --git a/bundler/lib/bundler/incomplete_specification.rb b/bundler/lib/bundler/incomplete_specification.rb new file mode 100644 index 000000000000..6d0b9b901c06 --- /dev/null +++ b/bundler/lib/bundler/incomplete_specification.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Bundler + class IncompleteSpecification + attr_reader :name, :platform + + def initialize(name, platform) + @name = name + @platform = platform + end + end +end diff --git a/bundler/lib/bundler/spec_set.rb b/bundler/lib/bundler/spec_set.rb index 4b99351f26ce..f0f9d093a092 100644 --- a/bundler/lib/bundler/spec_set.rb +++ b/bundler/lib/bundler/spec_set.rb @@ -31,7 +31,7 @@ def for(dependencies, check = false, platforms = [nil]) deps << [d.name, dep[1]] end elsif check - return false + specs << IncompleteSpecification.new(*dep) end end @@ -39,7 +39,7 @@ def for(dependencies, check = false, platforms = [nil]) specs << spec end - check ? true : specs + specs end def [](key) @@ -66,7 +66,7 @@ def to_hash end def materialize(deps) - materialized = self.for(deps, false).uniq + materialized = self.for(deps, true).uniq materialized.map! do |s| next s unless s.is_a?(LazySpecification) @@ -94,6 +94,10 @@ def missing_specs @specs.select {|s| s.is_a?(LazySpecification) } end + def incomplete_specs + @specs.select {|s| s.is_a?(IncompleteSpecification) } + end + def merge(set) arr = sorted.dup set.each do |set_spec| From ee0c9899625ceec824d3eb779c3e5daf1507bac2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Tue, 17 May 2022 13:24:13 +0200 Subject: [PATCH 5/7] Add an assertion to cover special handling of Bundler Since the improvement to lazily check whether the lockfile has missing platform specific dependencies, special handling on "bundler" inside `Bundler::SpecSet#for` is no longer covered by specs, because even in the case where missing platform specific dependencies are found, we still show the "Found no changes" message initially. This extra assertion makes the code covered again. --- bundler/spec/runtime/setup_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/bundler/spec/runtime/setup_spec.rb b/bundler/spec/runtime/setup_spec.rb index 033102f4e3bb..4e48aba6b686 100644 --- a/bundler/spec/runtime/setup_spec.rb +++ b/bundler/spec/runtime/setup_spec.rb @@ -635,6 +635,7 @@ def clean_load_path(lp) ruby "require '#{system_gem_path("gems/bundler-9.99.9.beta1/lib/bundler.rb")}'; Bundler.setup", :env => { "DEBUG" => "1" } expect(out).to include("Found no changes, using resolution from the lockfile") + expect(out).not_to include("lockfile does not have all gems needed for the current platform") expect(err).to be_empty end From 98ad5e5c8494b4f362567f28198db12854786dee Mon Sep 17 00:00:00 2001 From: Josh Nichols Date: Fri, 13 May 2022 17:22:54 -0400 Subject: [PATCH 6/7] Improve performance of Bundler::SpecSet#for by using hash lookup of handled deps I was looking at (yet another) flamegraph in speedscope, and used the 'left hand heavy' and was shocked to realize that 0.5s of the 1.7s is spent in DepProxy#name. This method _only_ delegates the name to an underlying spec, so it's not complex at all. It seems to be of how often this line ends up calling it: next if handled.any?{|d| d.name == dep.name && (match_current_platform || d.__platform == dep.__platform) } || dep.name == "bundler" The `handled` array is built up as dependencies are handled, so this get slower as more dependencies are installed. This change changes how `handled` is track. Instead of just an array, I've tried using a Hash, with the key being a dep's name, and the value being a list of deps with that name. This means it's constant time to find the dependencies with the same name. I saw a drop from 1.7s to 1.0s against master, and from 0.95s to 0.24s when used with https://github.com/rubygems/rubygems/pull/5533 --- bundler/lib/bundler/spec_set.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/bundler/lib/bundler/spec_set.rb b/bundler/lib/bundler/spec_set.rb index f0f9d093a092..93776d63d568 100644 --- a/bundler/lib/bundler/spec_set.rb +++ b/bundler/lib/bundler/spec_set.rb @@ -11,16 +11,18 @@ def initialize(specs) @specs = specs end - def for(dependencies, check = false, platforms = [nil]) - handled = ["bundler"].product(platforms) - deps = dependencies.map(&:name).product(platforms) + def for(dependencies, check = false, match_current_platform = false) + # dep.name => [list, of, deps] + handled = ["bundler"].product(platforms).map {|k| [k, true] }.to_h + deps = dependencies.dup specs = [] loop do break unless dep = deps.shift - next if handled.include?(dep) + next if handled.key?(dep) - handled << dep + # use a hash here to ensure constant lookup time in the `any?` call above + handled[dep] = true specs_for_dep = spec_for_dependency(*dep) if specs_for_dep.any? From 1ccf3994d15e77ab347651269adff173baf12966 Mon Sep 17 00:00:00 2001 From: Josh Nichols Date: Sun, 29 May 2022 11:06:14 -0400 Subject: [PATCH 7/7] Fix merge --- bundler/lib/bundler/spec_set.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundler/lib/bundler/spec_set.rb b/bundler/lib/bundler/spec_set.rb index 93776d63d568..9e03cbc1d75e 100644 --- a/bundler/lib/bundler/spec_set.rb +++ b/bundler/lib/bundler/spec_set.rb @@ -11,10 +11,10 @@ def initialize(specs) @specs = specs end - def for(dependencies, check = false, match_current_platform = false) + def for(dependencies, check = false, platforms = [nil]) # dep.name => [list, of, deps] handled = ["bundler"].product(platforms).map {|k| [k, true] }.to_h - deps = dependencies.dup + deps = dependencies.map(&:name).product(platforms) specs = [] loop do