Skip to content

Commit

Permalink
[rubygems/rubygems] Improve performance of Bundler::SpecSet#for by us…
Browse files Browse the repository at this point in the history
…ing 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 rubygems/rubygems#5533

rubygems/rubygems@844dac30d4
  • Loading branch information
technicalpickles authored and matzbot committed Jun 19, 2022
1 parent da362fe commit aeab405
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions lib/bundler/spec_set.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,17 @@ def initialize(specs)
end

def for(dependencies, check = false, match_current_platform = false)
handled = []
# dep.name => [list, of, deps]
handled = Hash.new {|h, k| h[k] = [] }
deps = dependencies.dup
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[dep.name].any? {|d| match_current_platform || d.__platform == dep.__platform } || dep.name == "bundler"

handled << dep
# use a hash here to ensure constant lookup time in the `any?` call above
handled[dep.name] << dep

specs_for_dep = spec_for_dependency(dep, match_current_platform)
if specs_for_dep.any?
Expand Down

0 comments on commit aeab405

Please sign in to comment.