Re-enable all the indexes / specs #73

greysteil · 2017-07-24T16:27:15Z

Gets Molinillo to the point where it can resolve anything that's thrown at it. Includes the following fixes and speedups (which will be pulled out into separate PRs when they're ready):

~~Handling failures where an unwind of a previous conflict is needed~~ (Consider previous conflicts when determining the state to unwind to #72)
~~Speeding up resolution by filtering grandparent+ states~~ (Filter grandparents and up of requirements when unwinding #77)
~~Handling circular dependencies~~ (Only raise CircularDependency errors if they prevent resolution #78)
Handling unwinds of a previous conflict for the same dependency is needed (this PR)

TODO:

Get specs passing in all known cases
- Testing locally should get no errors when running the RandomSortIndex specs 1000x
Improve performance while continuing to track all past conflicts (spec slow down is most noticeable on ReverseBundlerIndex, which is the place to start analysis...)
Clean up circular dependency handling (current special-casing feels grim)
Document approach in ARCHITECTURE.md where appropriate (speedup is undoubtedly going to require some hard thinking about how we loop through conflicts)
Spec thorny cases (~~circular dependencies~~, multiple states for a single requirement, rewinding on previous conflicts)

segiddins · 2017-07-24T19:49:55Z

lib/molinillo/resolution.rb

@@ -185,6 +195,9 @@ def process_topmost_state
          create_conflict if state.is_a? PossibilityState
          unwind_for_conflict until possibility && state.is_a?(DependencyState)
        end
+      rescue CircularDependencyError => root_error


I think this new behavior will require a test

For sure. CocoaPods/Resolver-Integration-Specs#14

Doesn't pass on every index

Does now. Still don't love the way I've implemented rescuing from CircularDependency errors, though. I'll have a think about it once I get properly handling previous conflicts working fast.

segiddins · 2017-07-25T19:06:12Z

lib/molinillo/resolution.rb

@@ -440,7 +467,7 @@ def parent_of(requirement)
      # @return [Object] the requirement that led to a version of a possibility
      #   with the given name being activated.
      def requirement_for_existing_name(name)
-        return nil unless activated.vertex_named(name).payload
+        return nil unless activated.vertex_named(name) && activated.vertex_named(name).payload


let's store the vertex into a local to avoid the second lookup

segiddins · 2017-07-25T19:06:24Z

lib/molinillo/resolution.rb

@@ -391,6 +417,7 @@ def binding_requirements_for_conflict(conflict)
        # then the only two requirements we need to consider are the initial one
        # (where the dependency's version was first chosen) and the last
        if binding_requirement_in_set?(nil, possible_binding_requirements, possibilities)
+          return possible_binding_requirements if conflict.root_error


a comment on this would be 🚀

segiddins · 2017-07-25T19:22:05Z

lib/molinillo/resolution.rb

@@ -453,7 +480,7 @@ def find_state_for(requirement)

      # @return [Conflict] a {Conflict} that reflects the failure to activate
      #   the {#possibility} in conjunction with the current {#state}
-      def create_conflict
+      def create_conflict(root_error = nil)


document the param

segiddins · 2017-07-25T19:41:30Z

lib/molinillo/resolution.rb

@@ -241,82 +259,78 @@ def initial_state
      # @return [void]
      def unwind_for_conflict
        details_for_unwind = build_details_for_unwind
+        tmp = state.unused_unwind_options


the state. is redundant

segiddins · 2017-07-25T19:42:17Z

lib/molinillo/resolution.rb

+        # for primary / parent elements of the tree?
+        alternative = unused_unwind_options.select do |option_set|
+          next false unless option_set.first > unwind_details.first
+          (option_set.map(&:requirement).flatten & unwind_details.map(&:requirement).flatten).any?


Compatibility.flat_map

segiddins · 2017-07-25T19:43:23Z

lib/molinillo/resolution.rb


        binding_requirements.reverse_each do |r|
+          unwind_details << UnwindDetails.new(-1, requirement_tree_for(r), nil, binding_requirements)


I think calculating the full requirement tree is rather slow... is it really necessary?

With the approach I'm experimenting with it is. Might be able to speed things up by doing it lazily, but I'm not sure yet.

High level, the idea here is that all previous unwind options remain possible later, even if they're not selected at first. To make that feasible, we need to:

Store them all (hence the removal of an early-exits from this method)

Pick the relevant ones when rewinding from a future conflict

It's part (2) that needs the requirement trees - to know whether a previous, unused rewind would avoid the current conflict, we want to check whether there's any overlap between that previous conflict's requirement tree (the amalgamation of the requirement trees for all the requirements that caused it) and the current conflict's tree.

Part I'm hazy about is exactly how much filtering we can do on the past rewinds, but I'm more comfortable working back from the above, which I'm relatively certain is formally correct, than trying to get to correctness from an "efficient" solution.

🤞

segiddins · 2017-07-25T19:45:16Z

lib/molinillo/resolution.rb

            end
            grandparent_r = parent_of(grandparent_r)
          end
        end

-        unwind_details
+        unwind_details.sort.reverse


might it be faster to do unwind_details.sort { |l ,r| r <=> l } to avoid the intermediary array?

segiddins · 2017-07-26T00:56:43Z

lib/molinillo/resolution.rb

@@ -324,18 +327,23 @@ def build_details_for_unwind
      #    resolve the given conflict conflict
      def unwind_details_for_requirements(binding_requirements)
        unwind_details = []
-
+        full_requirement_tree = binding_requirements.map { |r| requirement_tree_for(r) }.flatten


Please either flat map or flatten a specific amount

segiddins · 2017-07-26T18:58:48Z

lib/molinillo/resolution.rb

@@ -460,8 +461,12 @@ def filter_possibilities_for_primary_unwind(unwind_details)
      # @param [UnwindDetails] details of the conflict just unwound from
      # @return [void]
      def filter_possibilities_for_parent_unwind(unwind_details)
+        unwinds = [unwind_details] + unused_unwind_options.select { |uw| uw.state_index == unwind_details.state_index }


.select << unwind_details to save an array allocation

segiddins · 2017-07-26T19:10:37Z

lib/molinillo/resolution.rb

            end
            parent_r = grandparent_r
            grandparent_r = parent_of(parent_r)
          end
        end

-        unwind_details
+        unwind_details.sort


is it possible this array won't be used, and we could maybe sort! it when first accessed?

segiddins · 2017-07-26T19:21:05Z

lib/molinillo/resolution.rb

+          next unless alternative > current_detail
+          intersecting_requirements =
+            alternative.requirement_trees.flatten(1) &
+            last_detail_for_current_unwind.requirement_trees.flatten(1)


this can be pulled out into a local, but it might be worth memoizing this in the details?

greysteil · 2017-07-27T10:39:59Z

Crude benchmarking:

Running all specs on master 100 times: 329 seconds
This branch (with only masters' specs): 335 seconds

That should be well within any allowable margin of error.

🎉

segiddins

I'm so indescribably excited for this to land. Thank you so so much @greysteil, you've managed to push Molinillo forward when I thought I'd hit a wall with it.

segiddins · 2017-07-27T15:38:37Z

lib/molinillo/resolution.rb

+          intersecting_requirements =
+            last_detail_for_current_unwind.all_requirements &
+            alternative.requirements_unwound_to_instead
+          next unless intersecting_requirements.any?


next if empty?

segiddins · 2017-07-27T15:42:21Z

lib/molinillo/resolution.rb

+      # @param [Array] requirements an array of requirements
+      # @return [Boolean] whether the possibility satisfies all of the
+      #    given requirements
+      def possibility_satisfies_requirements?(possibility, requirements)


segiddins · 2017-07-27T15:44:52Z

spec/resolver_spec.rb

-        # These indexes don't do a great job sorting, and segiddins has been
-        # unable to get the test passing with the bad sort without breaking
-        # other specs
+        # This index occassionally finds orders that are *incredibly* slow to resolver,


segiddins · 2017-07-27T15:45:02Z

spec/spec_helper/index.rb

@@ -198,7 +198,7 @@ def versions_of(dependency_name)
    BundlerIndex,
    ReverseBundlerIndex,
    BundlerSingleAllNoPenaltyIndex,
-    # RandomSortIndex,
+    RandomSortIndex,


Previously, we kept a hash of previous conflicts in `DependencyState#conflicts`, and considered unwinds from those previous considering when determining which state to unwind to. Unfortunately, there were a couple of problems with that approach: - A maximum of one conflict per dependency was stored. When a conflict was "resolved" and a new version of that dependency was activated, the previous conflict was deleted. Doing so deleted unwind options that may have been used in the future. - The `DependencyState#conflicts` method was also used when constructing error messages, so our ability to tweak it's behaviour was limited. - Full requirement trees for the conflicting requirements, and the associated unwind options, weren't stored, and could not be recreated at a later date (since the DependencyGraph would have changed since the time when the conflict was created) This PR introduces a new store on each `DependencyState` which contains details of all previously unused unwinds. Those old, previously unused unwinds are then considered when deciding whether to unwind from a new conflict. In theory, an old unwind may allow us to avoid the conflict we've just encountered, if it would affect the conflict's requirement trees in any way.

greysteil · 2017-07-27T16:02:55Z

My pleasure @segiddins - it's an awesome project, and I've enjoyed every minute of working on it. (Well, maybe not all of the minutes banging my head against the keyboard...!)

greysteil force-pushed the more-unwinding-improvements branch 2 times, most recently from ed8a9ca to 2ab704c Compare July 24, 2017 19:39

segiddins reviewed Jul 24, 2017

View reviewed changes

greysteil mentioned this pull request Jul 24, 2017

Spec circular dependencies that only appear for one bad version CocoaPods/Resolver-Integration-Specs#14

Merged

segiddins force-pushed the more-unwinding-improvements branch from e51ccb0 to 2ab704c Compare July 24, 2017 21:44

greysteil force-pushed the more-unwinding-improvements branch 8 times, most recently from 1713d5d to 4a72214 Compare July 25, 2017 18:33

segiddins reviewed Jul 25, 2017

View reviewed changes

greysteil force-pushed the more-unwinding-improvements branch from df3c750 to b2950e0 Compare July 25, 2017 19:39

segiddins reviewed Jul 25, 2017

View reviewed changes

greysteil force-pushed the more-unwinding-improvements branch 5 times, most recently from 46b50a8 to 6135941 Compare July 25, 2017 20:52

segiddins reviewed Jul 26, 2017

View reviewed changes

greysteil mentioned this pull request Jul 26, 2017

Filter grandparents and up of requirements when unwinding #77

Merged

greysteil force-pushed the more-unwinding-improvements branch 2 times, most recently from 3e81076 to 85052bd Compare July 26, 2017 13:23

greysteil mentioned this pull request Jul 26, 2017

Add integration spec for CocoaPods/Molinillo/66 CocoaPods/Resolver-Integration-Specs#11

Merged

greysteil force-pushed the more-unwinding-improvements branch 3 times, most recently from 4581619 to 4e385c9 Compare July 26, 2017 18:29

Re-enable all the indexes / specs

cd27792

greysteil force-pushed the more-unwinding-improvements branch 3 times, most recently from 525f8c3 to 811cef5 Compare July 26, 2017 19:19

segiddins reviewed Jul 26, 2017

View reviewed changes

greysteil force-pushed the more-unwinding-improvements branch 5 times, most recently from 9d67ace to c92a601 Compare July 27, 2017 10:24

segiddins approved these changes Jul 27, 2017

View reviewed changes

greysteil added 4 commits July 27, 2017 16:51

Update architecture doc with new unwinding approach

e6238fb

Disable non-trivial unwinding case for RandomSortIndex

74324f9

Add changelog entry

5a58e39

greysteil force-pushed the more-unwinding-improvements branch from 8e0314c to 5a58e39 Compare July 27, 2017 16:00

greysteil changed the title ~~[WIP] Re-enable all the indexes / specs~~ Re-enable all the indexes / specs Jul 27, 2017

segiddins merged commit a5b9a73 into master Jul 27, 2017

segiddins deleted the more-unwinding-improvements branch July 27, 2017 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable all the indexes / specs #73

Re-enable all the indexes / specs #73

greysteil commented Jul 24, 2017 •

edited

Loading

segiddins Jul 24, 2017

greysteil Jul 24, 2017

segiddins Jul 24, 2017

greysteil Jul 24, 2017

segiddins Jul 25, 2017

segiddins Jul 25, 2017

segiddins Jul 25, 2017

segiddins Jul 25, 2017

segiddins Jul 25, 2017

segiddins Jul 25, 2017

greysteil Jul 25, 2017

segiddins Jul 25, 2017

segiddins Jul 26, 2017

segiddins Jul 26, 2017

segiddins Jul 26, 2017

segiddins Jul 26, 2017

greysteil commented Jul 27, 2017 •

edited

Loading

segiddins left a comment

segiddins Jul 27, 2017

segiddins Jul 27, 2017

segiddins Jul 27, 2017

segiddins Jul 27, 2017

greysteil commented Jul 27, 2017


		binding_requirements.reverse_each do \|r\|
		unwind_details << UnwindDetails.new(-1, requirement_tree_for(r), nil, binding_requirements)

Re-enable all the indexes / specs #73

Re-enable all the indexes / specs #73

Conversation

greysteil commented Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greysteil commented Jul 27, 2017 • edited Loading

segiddins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greysteil commented Jul 27, 2017

greysteil commented Jul 24, 2017 •

edited

Loading

greysteil commented Jul 27, 2017 •

edited

Loading