Skip to content

Expensive assertion in Distribution.Solver.Modular.Linking #4258

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.

Sign up for GitHub

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

Jump to bottom
Closed
grayjay opened this issue Jan 23, 2017 · 4 comments · Fixed by #4346
Closed

Expensive assertion in Distribution.Solver.Modular.Linking #4258

grayjay opened this issue Jan 23, 2017 · 4 comments · Fixed by #4346

Comments

@grayjay
Copy link
Collaborator

grayjay commented Jan 23, 2017

The assertion at

cabal/cabal-install/Distribution/Solver/Modular/Linking.hs

Line 138 in 6d993e2

assert (lgInvariant $ vsLinks st) $ return ()
makes the solver run slowly when there are many setup dependencies, especially with --reorder-goals. Solving for jsaddle-warp with new-build is a good example (See #4192 (comment)). When I ran cabal new-build jsaddle-warp --dry-run with GHC 8.0.1, it took almost nine minutes with the assertion and about eight seconds without.

I didn't see any obvious ways to improve the performance. However, even if we minimized the effect of assertions on runtime, assertions could still affect performance measurements during development. It's easy to accidentally compare a cabal executable with assertions enabled to one without assertions.

I'm not sure of the best solution. Here are some ideas:

  • Remove ghc-options: -fno-ignore-asserts from cabal.project and then enable assertions for unit tests in CI.
  • Add another build flag to control just the expensive assertions, and turn them on in CI. Other assertions would be enabled in cabal.project.
  • Print a warning when assertions are enabled.
@ezyang
Copy link
Contributor

ezyang commented Jan 23, 2017

The way that GHC's RTS handles expensive asserts is like (2): by default they are not turned on, but if you turn on a "sanity" mode it will start running with them. The RTS asserts are toggleable at runtime but that will probably be inconvenient to do in the solver unless we're threading around some solver environment (maybe we are?)

@23Skidoo
Copy link
Member

Print a warning when assertions are enabled.

We should definitely do that at least.

grayjay added a commit to grayjay/cabal that referenced this issue Feb 20, 2017
@grayjay
Control expensive assertions in cabal-install with a build flag.
95a2cdb 
I added a build flag, 'debug-assertions', and a function, 'debugAssert'.
'debugAssert' only calls 'assert' when the flag is enabled. I only replaced one
call to 'assert' so far (in Distribution.Solver.Modular.Linking) in order to
resolve haskell#4258.
grayjay added a commit to grayjay/cabal that referenced this issue Feb 20, 2017
@grayjay
Control expensive assertions in cabal-install with a build flag.
55354c1 
I added a function, 'debugAssert', that wraps 'assert' and only calls it when
the build flag 'debug-assertions' is enabled.  The flag defaults to false. I
only replaced one call to 'assert' so far (in Distribution.Solver.Modular.Linking)
in order to resolve haskell#4258.
grayjay added a commit to grayjay/cabal that referenced this issue Feb 20, 2017
@grayjay
Control expensive assertions in cabal-install with a build flag.
debc838 
I added a function, 'debugAssert', that wraps 'assert' and only calls it when
the build flag 'debug-assertions' is enabled.  The flag defaults to false. I
only replaced one call to 'assert' so far (in Distribution.Solver.Modular.Linking)
in order to resolve haskell#4258.
@grayjay grayjay mentioned this issue Feb 20, 2017
Merged
@grayjay
Copy link
Collaborator Author

grayjay commented Feb 20, 2017

Thanks for the feedback. I added a build flag in #4346 so that it would be easy to call the conditional assert from anywhere in cabal-install.

I think it would also be useful to warn when assertions are enabled. I'm not sure where we should put the warning, though.

23Skidoo pushed a commit that referenced this issue Feb 21, 2017
@grayjay @23Skidoo
Control expensive assertions in cabal-install with a build flag.
ab5257c 
I added a function, 'debugAssert', that wraps 'assert' and only calls it when
the build flag 'debug-assertions' is enabled.  The flag defaults to false. I
only replaced one call to 'assert' so far (in Distribution.Solver.Modular.Linking)
in order to resolve #4258.
grayjay added a commit to grayjay/cabal that referenced this issue Feb 22, 2017
@grayjay
Control expensive assertions in cabal-install with a build flag.
7a4f0c1 
I added a function, 'debugAssert', that wraps 'assert' and only calls it when
the build flag 'debug-assertions' is enabled.  The flag defaults to false. I
only replaced one call to 'assert' so far (in Distribution.Solver.Modular.Linking)
in order to resolve haskell#4258.

(cherry picked from commit ab5257c)
23Skidoo pushed a commit that referenced this issue Feb 22, 2017
@grayjay @23Skidoo
Control expensive assertions in cabal-install with a build flag.
b129a06 
I added a function, 'debugAssert', that wraps 'assert' and only calls it when
the build flag 'debug-assertions' is enabled.  The flag defaults to false. I
only replaced one call to 'assert' so far (in Distribution.Solver.Modular.Linking)
in order to resolve #4258.

(cherry picked from commit ab5257c)
@grayjay grayjay mentioned this issue Mar 6, 2017
Closed
@grayjay
Copy link
Collaborator Author

grayjay commented Mar 6, 2017

Print a warning when assertions are enabled.

We should definitely do that at least.

I opened a new issue: #4377

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Control expensive assertion in Distribution.Solver.Modular.Linking with a build flag. grayjay/cabal
3 participants
@ezyang @23Skidoo @grayjay