Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the package instance from D.Solver.Modular.Var (closes #4142). #4791

Merged
merged 2 commits into from
Sep 27, 2017

Conversation

grayjay
Copy link
Collaborator

@grayjay grayjay commented Sep 25, 2017

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!


Remove D.Solver.Modular.Var.varPI.

This change is necessary to remove the package instance (I) from Var
(issue #4142). One of the main uses of the package instance is when varPI is
called by the linking phase in order to get the dependencies introduced by flags
and stanzas. The validation phase also needs to look up dependencies introduced
by flags and stanzas, but it does so by looking up the dependencies once when it
chooses a package and then storing the dependencies in a map. I refactored the
linking phase to also store dependencies in a map.

Remove the package instance from D.Solver.Modular.Var (closes #4142).

This change has several effects:

  • The solver no longer includes the package version in messages that relate to
    a package's flags, stanzas, or dependencies. However, the solver always
    chooses the package version before choosing any flags, stanzas, or
    dependencies for the package, so it should be easy to find the version
    by looking earlier in the log.
  • In conflict counting, the solver treats flags with the same name in different
    versions of a package as the same flag. This change in the conflict counting
    heuristic can improve the solver's efficiency when the same flag causes the
    same conflicts in different versions of a package. The same applies to
    enabling tests or benchmarks.
  • Each flag or stanza can only appear once in a conflict set. This has no
    effect on behavior, but it simplifies the message containing the final
    conflict set.

Here is an example of the change in a log message. It only prints
hackage-server's version once, when it first chooses the package. The conflict
set also has one fewer variable, but that is probably due to the change in
conflict counting.

 Resolving dependencies...
 cabal: Could not resolve dependencies:
 trying: hackage-server-0.5.0 (user goal)
-trying: hackage-server-0.5.0:+build-hackage-build
-trying: unix-2.7.2.1/installed-2.7... (dependency of hackage-server-0.5.0
+trying: hackage-server:+build-hackage-build
+trying: unix-2.7.2.1/installed-2.7... (dependency of hackage-server
 +build-hackage-build)
-next goal: aeson (dependency of hackage-server-0.5.0 +build-hackage-build)
+next goal: aeson (dependency of hackage-server +build-hackage-build)
 rejecting: aeson-1.2.2.0, aeson-1.2.1.0, aeson-1.2.0.0, aeson-1.1.2.0,
 aeson-1.1.1.0, aeson-1.1.0.0, aeson-1.0.2.1, aeson-1.0.2.0, aeson-1.0.1.0,
 aeson-1.0.0.0, aeson-0.11.3.0, aeson-0.11.2.1, aeson-0.11.2.0, aeson-0.11.1.4,
 aeson-0.11.1.3, aeson-0.11.1.2, aeson-0.11.1.1, aeson-0.11.1.0,
 aeson-0.11.0.0, aeson-0.9.0.1, aeson-0.9.0.0, aeson-0.8.1.1, aeson-0.8.1.0,
 aeson-0.8.0.2, aeson-0.7.0.6, aeson-0.7.0.4, aeson-0.6.2.1, aeson-0.6.2.0
 (conflict: hackage-server +build-hackage-build => aeson==0.6.1.*)
 rejecting: aeson-0.6.1.0 (conflict: unix => time==1.6.0.1/installed-1.6...,
 aeson => time<1.5)
 rejecting: aeson-0.6.0.2, aeson-0.6.0.1, aeson-0.6.0.0, aeson-0.5.0.0,
 aeson-0.4.0.1, aeson-0.4.0.0, aeson-0.3.2.14, aeson-0.3.2.13, aeson-0.3.2.12,
 aeson-0.3.2.11, aeson-0.3.2.10, aeson-0.3.2.9, aeson-0.3.2.8, aeson-0.3.2.7,
 aeson-0.3.2.6, aeson-0.3.2.5, aeson-0.3.2.4, aeson-0.3.2.3, aeson-0.3.2.2,
 aeson-0.3.2.1, aeson-0.3.2.0, aeson-0.3.1.1, aeson-0.3.1.0, aeson-0.3.0.0,
 aeson-0.2.0.0, aeson-0.1.0.0, aeson-0.10.0.0, aeson-0.8.0.1, aeson-0.8.0.0,
 aeson-0.7.0.5, aeson-0.7.0.3, aeson-0.7.0.2, aeson-0.7.0.1, aeson-0.7.0.0
 (conflict: hackage-server +build-hackage-build => aeson==0.6.1.*)
 After searching the rest of the dependency tree exhaustively, these were the
-goals I've had most trouble fulfilling: aeson, hackage-server,
-hackage-server-0.5.0:build-hackage-build,
-hackage-server-0.4:build-hackage-mirror, template-haskell
+goals I've had most trouble fulfilling: aeson,
+hackage-server:build-hackage-build, hackage-server, template-haskell

I ran hackage-benchmark to compare this commit with master (two commits
earlier). I used --min-run-time-percentage-difference-to-rerun=10 to only rerun
packages if the run times differed by more than 10% in the first trial, and
defaults for the rest of the options (10 trials, p-value of 0.05, 90 second
timeout). The index state was "2017-09-24T03:35:06Z".

1 is master, and 2 is this commit:

package result1 result2 mean1 mean2 stddev1 stddev2 speedup
CC-delcont-ref Solution Solution 1.467s 1.505s 0.019s 0.100s 0.975
ascii-cows Solution Solution 1.827s 1.758s 0.159s 0.012s 1.040
opaleye-classy NoInstallPlan NoInstallPlan 4.588s 4.070s 0.043s 0.032s 1.127
range-space NoInstallPlan NoInstallPlan 2.642s 2.299s 0.016s 0.016s 1.149
rts PkgNotFound PkgNotFound 1.323s 1.327s 0.032s 0.033s 0.997
servant-auth-docs Solution Solution 1.968s 1.998s 0.017s 0.074s 0.985
thorn BackjumpLimit NoInstallPlan 4.793s 3.141s 0.050s 0.034s 1.526
unordered-intmap Solution Solution 1.502s 1.511s 0.081s 0.047s 0.994

I looked at the solver logs for the three packages with the largest changes in
run time, opaleye-classy, range-space, and thorn. Each one showed that the
solver started preferring a flag in an older version of a package after it had
caused conflicts in a newer version of the package.

/cc @kosmikus

This change is necessary to remove the package instance (I) from Var
(issue haskell#4142).  One of the main uses of the package instance is when varPI is
called by the linking phase in order to get the dependencies introduced by flags
and stanzas.  The validation phase also needs to look up dependencies introduced
by flags and stanzas, but it does so by looking up the dependencies once when it
chooses a package and then storing the dependencies in a map.  I refactored the
linking phase to also store dependencies in a map.
…#4142).

This change has several effects:

- The solver no longer includes the package version in messages that relate to
  a package's flags, stanzas, or dependencies.  However, the solver always
  chooses the package version before choosing any flags, stanzas, or
  dependencies for the package, so it should be easy to find the version
  by looking earlier in the log.
- In conflict counting, the solver treats flags with the same name in different
  versions of a package as the same flag.  This change in the conflict counting
  heuristic can improve the solver's efficiency when the same flag causes the
  same conflicts in different versions of a package.  The same applies to
  enabling tests or benchmarks.
- Each flag or stanza can only appear once in a conflict set.  This has no
  effect on behavior, but it simplifies the message containing the final
  conflict set.

Here is an example of the change in a log message.  It only prints
hackage-server's version once, when it first chooses the package.  The conflict
set also has one fewer variable, but that is probably due to the change in
conflict counting.

 Resolving dependencies...
 cabal: Could not resolve dependencies:
 trying: hackage-server-0.5.0 (user goal)
-trying: hackage-server-0.5.0:+build-hackage-build
-trying: unix-2.7.2.1/installed-2.7... (dependency of hackage-server-0.5.0
+trying: hackage-server:+build-hackage-build
+trying: unix-2.7.2.1/installed-2.7... (dependency of hackage-server
 +build-hackage-build)
-next goal: aeson (dependency of hackage-server-0.5.0 +build-hackage-build)
+next goal: aeson (dependency of hackage-server +build-hackage-build)
 rejecting: aeson-1.2.2.0, aeson-1.2.1.0, aeson-1.2.0.0, aeson-1.1.2.0,
 aeson-1.1.1.0, aeson-1.1.0.0, aeson-1.0.2.1, aeson-1.0.2.0, aeson-1.0.1.0,
 aeson-1.0.0.0, aeson-0.11.3.0, aeson-0.11.2.1, aeson-0.11.2.0, aeson-0.11.1.4,
 aeson-0.11.1.3, aeson-0.11.1.2, aeson-0.11.1.1, aeson-0.11.1.0,
 aeson-0.11.0.0, aeson-0.9.0.1, aeson-0.9.0.0, aeson-0.8.1.1, aeson-0.8.1.0,
 aeson-0.8.0.2, aeson-0.7.0.6, aeson-0.7.0.4, aeson-0.6.2.1, aeson-0.6.2.0
 (conflict: hackage-server +build-hackage-build => aeson==0.6.1.*)
 rejecting: aeson-0.6.1.0 (conflict: unix => time==1.6.0.1/installed-1.6...,
 aeson => time<1.5)
 rejecting: aeson-0.6.0.2, aeson-0.6.0.1, aeson-0.6.0.0, aeson-0.5.0.0,
 aeson-0.4.0.1, aeson-0.4.0.0, aeson-0.3.2.14, aeson-0.3.2.13, aeson-0.3.2.12,
 aeson-0.3.2.11, aeson-0.3.2.10, aeson-0.3.2.9, aeson-0.3.2.8, aeson-0.3.2.7,
 aeson-0.3.2.6, aeson-0.3.2.5, aeson-0.3.2.4, aeson-0.3.2.3, aeson-0.3.2.2,
 aeson-0.3.2.1, aeson-0.3.2.0, aeson-0.3.1.1, aeson-0.3.1.0, aeson-0.3.0.0,
 aeson-0.2.0.0, aeson-0.1.0.0, aeson-0.10.0.0, aeson-0.8.0.1, aeson-0.8.0.0,
 aeson-0.7.0.5, aeson-0.7.0.3, aeson-0.7.0.2, aeson-0.7.0.1, aeson-0.7.0.0
 (conflict: hackage-server +build-hackage-build => aeson==0.6.1.*)
 After searching the rest of the dependency tree exhaustively, these were the
-goals I've had most trouble fulfilling: aeson, hackage-server,
-hackage-server-0.5.0:build-hackage-build,
-hackage-server-0.4:build-hackage-mirror, template-haskell
+goals I've had most trouble fulfilling: aeson,
+hackage-server:build-hackage-build, hackage-server, template-haskell

I ran hackage-benchmark to compare this commit with master (two commits
earlier).  I used --min-run-time-percentage-difference-to-rerun=10 to only rerun
packages if the run times differed by more than 10% in the first trial, and
defaults for the rest of the options (10 trials, p-value of 0.05, 90 second
timeout). The index state was "2017-09-24T03:35:06Z".

1 is master, and 2 is this commit:

package            result1       result2             mean1       mean2     stddev1     stddev2     speedup
CC-delcont-ref     Solution      Solution           1.467s      1.505s      0.019s      0.100s      0.975
ascii-cows         Solution      Solution           1.827s      1.758s      0.159s      0.012s      1.040
opaleye-classy     NoInstallPlan NoInstallPlan      4.588s      4.070s      0.043s      0.032s      1.127
range-space        NoInstallPlan NoInstallPlan      2.642s      2.299s      0.016s      0.016s      1.149
rts                PkgNotFound   PkgNotFound        1.323s      1.327s      0.032s      0.033s      0.997
servant-auth-docs  Solution      Solution           1.968s      1.998s      0.017s      0.074s      0.985
thorn              BackjumpLimit NoInstallPlan      4.793s      3.141s      0.050s      0.034s      1.526
unordered-intmap   Solution      Solution           1.502s      1.511s      0.081s      0.047s      0.994

I looked at the solver logs for the three packages with the largest changes in
run time, opaleye-classy, range-space, and thorn.  Each one showed that the
solver started preferring a flag in an older version of a package after it had
caused conflicts in a newer version of the package.
@mention-bot
Copy link

@grayjay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kosmikus, @edsko and @Ericson2314 to be potential reviewers.

@23Skidoo
Copy link
Member

AFAICT, this is good to go, though would be nice if @kosmikus could take a look.

@23Skidoo
Copy link
Member

23Skidoo commented Sep 27, 2017

I think I'll just merge this, looks uncontroversial enough and @kosmikus approved of the general idea in #4142.

@23Skidoo 23Skidoo merged commit 989676e into haskell:master Sep 27, 2017
@kosmikus
Copy link
Contributor

Yes, LGTM.

@grayjay
Copy link
Collaborator Author

grayjay commented Sep 27, 2017

Thanks!

@grayjay grayjay deleted the remove-PI-from-Var branch September 27, 2017 15:35
grayjay added a commit to grayjay/cabal that referenced this pull request Jan 8, 2018
grayjay added a commit to grayjay/cabal that referenced this pull request Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we remove the package version from flag/stanza variables in the solver?
4 participants