Refactor the DG

CHANGELOG.md

@@ -1,5 +1,25 @@
 # Molinillo Changelog
 
+## Master
+
+##### API Breaking Changes
+
+* The `DependencyGraph` no longer treats root vertices specially, nor does it
+  maintain a direct reference to `edges`. Additionally, `Vertex` no longer
+  has a reference to its parent graph.  
+
+##### Enhancements
+
+* Resolution has been sped up by 25x in some pathological cases, and in general
+  recursive operations on a `DependencyGraph` or `Vertex` are now `O(n)`.  
+  [Samuel Giddins](https://github.com/segiddins)
+  [Bundler#3803](https://github.com/bundler/bundler/issues/3803)
+
+* Re-sorting of dependencies is skipped when the unresolved dependency list has
+  not changed, speeding up resolution of fully locked graphs.  
+  [Samuel Giddins](https://github.com/segiddins)
+
+
 ## 0.3.1
 
 ##### Enhancements

lib/molinillo/dependency_graph.rb

@@ -34,40 +34,34 @@ def self.tsort(vertices)
     # A directed edge of a {DependencyGraph}
     # @attr [Vertex] origin The origin of the directed edge
     # @attr [Vertex] destination The destination of the directed edge
-    # @attr [Array] requirements The requirements the directed edge represents
-    Edge = Struct.new(:origin, :destination, :requirements)
+    # @attr [Object] requirement The requirement the directed edge represents
+    Edge = Struct.new(:origin, :destination, :requirement)
 
-    # @return [{String => Vertex}] vertices that have no {Vertex#predecessors},
-    #   keyed by by {Vertex#name}
-    attr_reader :root_vertices
     # @return [{String => Vertex}] the vertices of the dependency graph, keyed
     #   by {Vertex#name}
     attr_reader :vertices
-    # @return [Set<Edge>] the edges of the dependency graph
-    attr_reader :edges
 
     def initialize
       @vertices = {}
-      @edges = Set.new
-      @root_vertices = {}
     end
 
     # Initializes a copy of a {DependencyGraph}, ensuring that all {#vertices}
-    # have the correct {Vertex#graph} set
+    # are properly copied.
     def initialize_copy(other)
       super
-      @vertices = other.vertices.reduce({}) do |vertices, (name, vertex)|
-        vertices.tap do |hash|
-          hash[name] = vertex.dup.tap { |v| v.graph = self }
+      @vertices = {}
+      traverse = lambda do |new_v, old_v|
+        return if new_v.outgoing_edges.size == old_v.outgoing_edges.size
+        old_v.outgoing_edges.each do |edge|
+          destination = add_vertex(edge.destination.name, edge.destination.payload)
+          add_edge_no_circular(new_v, destination, edge.requirement)
+          traverse.call(destination, edge.destination)
         end
       end
-      @root_vertices = Hash[@vertices.select { |n, _v| other.root_vertices[n] }]
-      @edges = other.edges.map do |edge|
-        Edge.new(
-          vertex_named(edge.origin.name),
-          vertex_named(edge.destination.name),
-          edge.requirements.dup
-        )
+      other.vertices.each do |name, vertex|
+        new_vertex = add_vertex(name, vertex.payload, vertex.root?)
+        new_vertex.explicit_requirements.replace(vertex.explicit_requirements)
+        traverse.call(new_vertex, vertex)
       end
     end
 
@@ -80,7 +74,12 @@ def inspect
     #   by a recursive traversal of each {#root_vertices} and its
     #   {Vertex#successors}
     def ==(other)
-      root_vertices == other.root_vertices
+      return false unless other
+      vertices.each do |name, vertex|
+        other_vertex = other.vertex_named(name)
+        return false unless other_vertex
+        return false unless other_vertex.successors.map(&:name).to_set == vertex.successors.map(&:name).to_set
+      end
     end
 
     # @param [String] name
@@ -89,15 +88,13 @@ def ==(other)
     # @param [Object] requirement the requirement that is requiring the child
     # @return [void]
     def add_child_vertex(name, payload, parent_names, requirement)
-      is_root = parent_names.include?(nil)
-      parent_nodes = parent_names.compact.map { |n| vertex_named(n) }
-      vertex = vertex_named(name) || if is_root
-                                       add_root_vertex(name, payload)
-                                     else
-                                       add_vertex(name, payload)
-                                     end
-      vertex.payload ||= payload
-      parent_nodes.each do |parent_node|
+      vertex = add_vertex(name, payload)
+      parent_names.each do |parent_name|
+        unless parent_name
+          vertex.root = true
+          next
+        end
+        parent_node = vertex_named(parent_name)
         add_edge(parent_node, vertex, requirement)
       end
       vertex
@@ -106,29 +103,24 @@ def add_child_vertex(name, payload, parent_names, requirement)
     # @param [String] name
     # @param [Object] payload
     # @return [Vertex] the vertex that was added to `self`
-    def add_vertex(name, payload)
-      vertex = vertices[name] ||= Vertex.new(self, name, payload)
-      vertex.tap { |v| v.payload = payload }
-    end
-
-    # @param [String] name
-    # @param [Object] payload
-    # @return [Vertex] the vertex that was added to `self`
-    def add_root_vertex(name, payload)
-      add_vertex(name, payload).tap { |v| root_vertices[name] = v }
+    def add_vertex(name, payload, root = false)
+      vertex = vertices[name] ||= Vertex.new(name, payload)
+      vertex.payload ||= payload
+      vertex.root ||= root
+      vertex
     end
 
     # Detaches the {#vertex_named} `name` {Vertex} from the graph, recursively
     # removing any non-root vertices that were orphaned in the process
     # @param [String] name
     # @return [void]
     def detach_vertex_named(name)
-      vertex = vertex_named(name)
-      return unless vertex
-      successors = vertex.successors
-      vertices.delete(name)
-      edges.reject! { |e| e.origin == vertex || e.destination == vertex }
-      successors.each { |v| detach_vertex_named(v.name) unless root_vertices[v.name] || v.predecessors.any? }
+      return unless vertex = vertices.delete(name)
+      vertex.outgoing_edges.each do |e|
+        v = e.destination
+        v.incoming_edges.delete(e)
+        detach_vertex_named(v.name) unless v.root? || v.predecessors.any?
+      end
     end
 
     # @param [String] name
@@ -140,7 +132,8 @@ def vertex_named(name)
     # @param [String] name
     # @return [Vertex,nil] the root vertex with the given name
     def root_vertex_named(name)
-      root_vertices[name]
+      vertex = vertex_named(name)
+      vertex if vertex && vertex.root?
     end
 
     # Adds a new {Edge} to the dependency graph
@@ -149,18 +142,24 @@ def root_vertex_named(name)
     # @param [Object] requirement the requirement that this edge represents
     # @return [Edge] the added edge
     def add_edge(origin, destination, requirement)
-      if origin == destination || destination.path_to?(origin)
+      if destination.path_to?(origin)
         raise CircularDependencyError.new([origin, destination])
       end
-      Edge.new(origin, destination, [requirement]).tap { |e| edges << e }
+      add_edge_no_circular(origin, destination, requirement)
+    end
+
+    private
+
+    def add_edge_no_circular(origin, destination, requirement)
+      edge = Edge.new(origin, destination, requirement)
+      origin.outgoing_edges << edge
+      destination.incoming_edges << edge
+      edge
     end
 
     # A vertex in a {DependencyGraph} that encapsulates a {#name} and a
     # {#payload}
     class Vertex
-      # @return [DependencyGraph] the graph this vertex is a node of
-      attr_accessor :graph
-
       # @return [String] the name of the vertex
       attr_accessor :name
 
@@ -171,50 +170,62 @@ class Vertex
       #   this vertex
       attr_reader :explicit_requirements
 
-      # @param [DependencyGraph] graph see {#graph}
+      # @return [Boolean] whether the vertex is considered a root vertex
+      attr_accessor :root
+      alias_method :root?, :root
+
       # @param [String] name see {#name}
       # @param [Object] payload see {#payload}
-      def initialize(graph, name, payload)
-        @graph = graph
+      def initialize(name, payload)
         @name = name
         @payload = payload
         @explicit_requirements = []
+        @outgoing_edges = []
+        @incoming_edges = []
       end
 
       # @return [Array<Object>] all of the requirements that required
       #   this vertex
       def requirements
-        incoming_edges.map(&:requirements).flatten + explicit_requirements
+        incoming_edges.map(&:requirement) + explicit_requirements
       end
 
       # @return [Array<Edge>] the edges of {#graph} that have `self` as their
       #   {Edge#origin}
-      def outgoing_edges
-        graph.edges.select { |e| e.origin.shallow_eql?(self) }
-      end
+      attr_accessor :outgoing_edges
 
       # @return [Array<Edge>] the edges of {#graph} that have `self` as their
       #   {Edge#destination}
-      def incoming_edges
-        graph.edges.select { |e| e.destination.shallow_eql?(self) }
-      end
+      attr_accessor :incoming_edges
 
-      # @return [Set<Vertex>] the vertices of {#graph} that have an edge with
+      # @return [Array<Vertex>] the vertices of {#graph} that have an edge with
       #   `self` as their {Edge#destination}
       def predecessors
-        incoming_edges.map(&:origin).to_set
+        incoming_edges.map(&:origin)
+      end
+
+      # @return [Array<Vertex>] the vertices of {#graph} where `self` is a
+      #   {#descendent?}
+      def recursive_predecessors
+        vertices = predecessors
+        vertices += vertices.map(&:recursive_predecessors)
+        vertices.uniq!
+        vertices
       end
 
-      # @return [Set<Vertex>] the vertices of {#graph} that have an edge with
+      # @return [Array<Vertex>] the vertices of {#graph} that have an edge with
       #   `self` as their {Edge#origin}
       def successors
-        outgoing_edges.map(&:destination).to_set
+        outgoing_edges.map(&:destination)
       end
 
-      # @return [Set<Vertex>] the vertices of {#graph} where `self` is an
+      # @return [Array<Vertex>] the vertices of {#graph} where `self` is an
       #   {#ancestor?}
       def recursive_successors
-        successors + successors.map(&:recursive_successors).reduce(Set.new, &:+)
+        vertices = successors
+        vertices += vertices.map(&:recursive_successors)
+        vertices.uniq!
+        vertices
       end
 
       # @return [String] a string suitable for debugging
@@ -226,7 +237,7 @@ def inspect
       #   by a recursive traversal of each {Vertex#successors}
       def ==(other)
         shallow_eql?(other) &&
-          successors == other.successors
+          successors.to_set == other.successors.to_set
       end
 
       # @return [Boolean] whether the two vertices are equal, determined
@@ -248,7 +259,7 @@ def hash
       # dependency graph?
       # @return true iff there is a path following edges within this {#graph}
       def path_to?(other)
-        successors.include?(other) || successors.any? { |v| v.path_to?(other) }
+        equal?(other) || successors.any? { |v| v.path_to?(other) }
       end
 
       alias_method :descendent?, :path_to?
@@ -257,7 +268,7 @@ def path_to?(other)
       # dependency graph?
       # @return true iff there is a path following edges within this {#graph}
       def ancestor?(other)
-        predecessors.include?(other) || predecessors.any? { |v| v.ancestor?(other) }
+        other.path_to?(self)
       end
 
       alias_method :is_reachable_from?, :ancestor?

lib/molinillo/resolution.rb

@@ -166,7 +166,7 @@ def state
       # @return [DependencyState] the initial state for the resolution
       def initial_state
         graph = DependencyGraph.new.tap do |dg|
-          original_requested.each { |r| dg.add_root_vertex(name_for(r), nil).tap { |v| v.explicit_requirements << r } }
+          original_requested.each { |r| dg.add_vertex(name_for(r), nil, true).tap { |v| v.explicit_requirements << r } }
         end
 
         requirements = sort_dependencies(original_requested, graph, {})
@@ -252,7 +252,7 @@ def create_conflict
           name_for_explicit_dependency_source => vertex.explicit_requirements,
           name_for_locking_dependency_source => Array(locked_requirement_named(name)),
         }
-        vertex.incoming_edges.each { |edge| (requirements[edge.origin.payload] ||= []).unshift(*edge.requirements) }
+        vertex.incoming_edges.each { |edge| (requirements[edge.origin.payload] ||= []).unshift(edge.requirement) }
         conflicts[name] = Conflict.new(
           requirement,
           Hash[requirements.select { |_, r| !r.empty? }],
@@ -326,7 +326,7 @@ def attempt_to_activate_existing_spec(existing_node)
         existing_spec = existing_node.payload
         if requirement_satisfied_by?(requirement, activated, existing_spec)
           new_requirements = requirements.dup
-          push_state_for_requirements(new_requirements)
+          push_state_for_requirements(new_requirements, false)
         else
           return if attempt_to_swap_possibility
           create_conflict
@@ -393,17 +393,17 @@ def activate_spec
       def require_nested_dependencies_for(activated_spec)
         nested_dependencies = dependencies_for(activated_spec)
         debug(depth) { "Requiring nested dependencies (#{nested_dependencies.map(&:to_s).join(', ')})" }
-        nested_dependencies.each { |d|  activated.add_child_vertex name_for(d), nil, [name_for(activated_spec)], d }
+        nested_dependencies.each { |d| activated.add_child_vertex(name_for(d), nil, [name_for(activated_spec)], d) }
 
-        push_state_for_requirements(requirements + nested_dependencies)
+        push_state_for_requirements(requirements + nested_dependencies, nested_dependencies.size > 0)
       end
 
       # Pushes a new {DependencyState} that encapsulates both existing and new
       # requirements
       # @param [Array] new_requirements
       # @return [void]
-      def push_state_for_requirements(new_requirements, new_activated = activated.dup)
-        new_requirements = sort_dependencies(new_requirements.uniq, new_activated, conflicts)
+      def push_state_for_requirements(new_requirements, requires_sort = true, new_activated = activated.dup)
+        new_requirements = sort_dependencies(new_requirements.uniq, new_activated, conflicts) if requires_sort
         new_requirement = new_requirements.shift
         new_name = new_requirement ? name_for(new_requirement) : ''
         possibilities = new_requirement ? search_for(new_requirement) : []
@@ -424,7 +424,7 @@ def push_state_for_requirements(new_requirements, new_activated = activated.dup)
       def handle_missing_or_push_dependency_state(state)
         if state.requirement && state.possibilities.empty? && allow_missing?(state.requirement)
           state.activated.detach_vertex_named(state.name)
-          push_state_for_requirements(state.requirements, state.activated)
+          push_state_for_requirements(state.requirements.dup, false, state.activated)
         else
           states.push state
         end