Merge pull request #6931 from rubygems/martinemde/fetch_names_never_overrides

Improve Bundle::Index efficiency by removing unnecessary creation and dups

(cherry picked from commit 0e54343)

Author: martinemde

bundler/lib/bundler/index.rb

@@ -10,8 +10,8 @@ def self.build
       i
     end
 
-    attr_reader :specs, :all_specs, :sources
-    protected :specs, :all_specs
+    attr_reader :specs, :duplicates, :sources
+    protected :specs, :duplicates
 
     RUBY = "ruby"
     NULL = "\0"
@@ -20,20 +20,20 @@ def initialize
       @sources = []
       @cache = {}
       @specs = Hash.new {|h, k| h[k] = {} }
-      @all_specs = Hash.new {|h, k| h[k] = EMPTY_SEARCH }
+      @duplicates = {}
     end
 
     def initialize_copy(o)
       @sources = o.sources.dup
       @cache = {}
       @specs = Hash.new {|h, k| h[k] = {} }
-      @all_specs = Hash.new {|h, k| h[k] = EMPTY_SEARCH }
+      @duplicates = {}
 
       o.specs.each do |name, hash|
         @specs[name] = hash.dup
       end
-      o.all_specs.each do |name, array|
-        @all_specs[name] = array.dup
+      o.duplicates.each do |name, array|
+        @duplicates[name] = array.dup
       end
     end
 
@@ -47,7 +47,9 @@ def empty?
     end
 
     def search_all(name)
-      all_matches = local_search(name) + @all_specs[name]
+      all_matches = specs_by_name(name) # always returns a new Array
+      dupes = @duplicates[name]
+      all_matches.concat(dupes) if dupes
       @sources.each do |source|
         all_matches.concat(source.search_all(name))
       end
@@ -78,10 +80,11 @@ def local_search(query)
 
     alias_method :[], :search
 
-    def <<(spec)
+    def add(spec)
       @specs[spec.name][spec.full_name] = spec
       spec
     end
+    alias_method :<<, :add
 
     def each(&blk)
       return enum_for(:each) unless blk
@@ -115,15 +118,25 @@ def dependency_names
       names.uniq
     end
 
-    def use(other, override_dupes = false)
+    # Combines indexes proritizing existing specs, like `Hash#reverse_merge!`
+    # Duplicate specs found in `other` are stored in `@duplicates`.
+    def use(other)
       return unless other
-      other.each do |s|
-        if (dupes = search_by_spec(s)) && !dupes.empty?
-          # safe to << since it's a new array when it has contents
-          @all_specs[s.name] = dupes << s
-          next unless override_dupes
+      other.each do |spec|
+        exist?(spec) ? add_duplicate(spec) : add(spec)
+      end
+      self
+    end
+
+    # Combines indexes proritizing specs from `other`, like `Hash#merge!`
+    # Duplicate specs found in `self` are saved in `@duplicates`.
+    def merge!(other)
+      return unless other
+      other.each do |spec|
+        if existing = find_by_spec(spec)
+          add_duplicate(existing)
         end
-        self << s
+        add spec
       end
       self
     end
@@ -157,6 +170,10 @@ def add_source(index)
 
     private
 
+    def add_duplicate(spec)
+      (@duplicates[spec.name] ||= []) << spec
+    end
+
     def specs_by_name_and_version(name, version)
       specs_by_name(name).select {|spec| spec.version == version }
     end
@@ -168,8 +185,16 @@ def specs_by_name(name)
     EMPTY_SEARCH = [].freeze
 
     def search_by_spec(spec)
-      spec = @specs[spec.name][spec.full_name]
+      spec = find_by_spec(spec)
       spec ? [spec] : EMPTY_SEARCH
     end
+
+    def find_by_spec(spec)
+      @specs[spec.name][spec.full_name]
+    end
+
+    def exist?(spec)
+      @specs[spec.name].key?(spec.full_name)
+    end
   end
 end

bundler/lib/bundler/source/rubygems.rb

@@ -128,12 +128,12 @@ def identifier
       def specs
         @specs ||= begin
           # remote_specs usually generates a way larger Index than the other
-          # sources, and large_idx.use small_idx is way faster than
-          # small_idx.use large_idx.
-          idx = @allow_remote ? remote_specs.dup : Index.new
-          idx.use(cached_specs, :override_dupes) if @allow_cached || @allow_remote
-          idx.use(installed_specs, :override_dupes) if @allow_local
-          idx
+          # sources, and large_idx.merge! small_idx is way faster than
+          # small_idx.merge! large_idx.
+          index = @allow_remote ? remote_specs.dup : Index.new
+          index.merge!(cached_specs) if @allow_cached || @allow_remote
+          index.merge!(installed_specs) if @allow_local
+          index
         end
       end
 
@@ -274,9 +274,9 @@ def double_check_for(unmet_dependency_names)
 
         Bundler.ui.debug "Double checking for #{unmet_dependency_names || "all specs (due to the size of the request)"} in #{self}"
 
-        fetch_names(api_fetchers, unmet_dependency_names, remote_specs, false)
+        fetch_names(api_fetchers, unmet_dependency_names, remote_specs)
 
-        specs.use(remote_specs, false)
+        specs.use remote_specs
       end
 
       def dependency_names_to_double_check
@@ -380,7 +380,7 @@ def installed_specs
 
       def cached_specs
         @cached_specs ||= begin
-          idx = @allow_local ? installed_specs.dup : Index.new
+          idx = Index.new
 
           Dir["#{cache_path}/*.gem"].each do |gemfile|
             s ||= Bundler.rubygems.spec_from_gem(gemfile)
@@ -401,22 +401,22 @@ def remote_specs
           index_fetchers = fetchers - api_fetchers
 
           if index_fetchers.empty?
-            fetch_names(api_fetchers, dependency_names, idx, false)
+            fetch_names(api_fetchers, dependency_names, idx)
           else
-            fetch_names(fetchers, nil, idx, false)
+            fetch_names(fetchers, nil, idx)
           end
         end
       end
 
-      def fetch_names(fetchers, dependency_names, index, override_dupes)
+      def fetch_names(fetchers, dependency_names, index)
         fetchers.each do |f|
           if dependency_names
             Bundler.ui.info "Fetching gem metadata from #{URICredentialsFilter.credential_filtered_uri(f.uri)}", Bundler.ui.debug?
-            index.use f.specs_with_retry(dependency_names, self), override_dupes
+            index.use f.specs_with_retry(dependency_names, self)
             Bundler.ui.info "" unless Bundler.ui.debug? # new line now that the dots are over
           else
             Bundler.ui.info "Fetching source index from #{URICredentialsFilter.credential_filtered_uri(f.uri)}"
-            index.use f.specs_with_retry(nil, self), override_dupes
+            index.use f.specs_with_retry(nil, self)
           end
         end
       end