Add test suite with two basic tests for solver space leaks. #4122
Conversation
This is an edge case. If there is only ever one choice for the next goal, the solver never evaluates the conflict count map. Additions to the map create a growing thunk.
|
@grayjay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @BardurArantsson, @fmthoma and @kosmikus to be potential reviewers. |
There was a problem hiding this comment.
Have you considered also limiting the stack size? Some space leaks tend to only build up the stack, but not the heap, and these would not be discovered here. (Example: foldr (+) 0 [1..])
And just for curiosity: Why did you disable backjumping for your test cases? Would it make sense to include additional test cases where backjumping is enabled?
| @@ -60,7 +60,7 @@ backjump (EnableBackjumping enableBj) var initial xs = | |||
| | otherwise = f (csAcc `CS.union` cs) cm' | |||
|
|
|||
| logBackjump :: ConflictSet QPN -> ConflictMap -> ConflictSetLog a | |||
| logBackjump cs cm = failWith (Failure cs Backjump) (cs, updateCM initial cm) | |||
| logBackjump cs cm = cm `seq` failWith (Failure cs Backjump) (cs, updateCM initial cm) | |||
There was a problem hiding this comment.
I'd consider a bang pattern instead of seq for readability.
Also, in exploreLog below the cm is only updated when conflict counting is enabled, while it is always updated here. As you suggested in #3960, I think the if countConflicts… would make sense here as well. (Or, if you think that seqing cm is already sufficient, remove the if countConflicts in exploreLog as well, which would IMHO improve readability).
There was a problem hiding this comment.
I'd consider a bang pattern instead of seq for readability.
fixed
Also, in exploreLog below the cm is only updated when conflict counting is enabled, while it is always updated here. As you suggested in #3960, I think the if countConflicts… would make sense here as well. (Or, if you think that seqing cm is already sufficient, remove the if countConflicts in exploreLog as well, which would IMHO improve readability).
I just made a minimal change to avoid conflicts with your PR, but you're right: it's simpler to always update the map.
|
@fmthoma Thanks for the review. I applied your suggestions in the last two commits.
Good idea. It seems to work really well, because the test framework catches the exception and continues running the other tests. Maybe we should also apply it to the other test suites.
I thought it would be better to test with backjumping, but I didn't know how to ensure that the solver doesn't find the result immediately. Then the tests wouldn't catch space leaks that only appear after a large area of the search tree has been explored. Turning off backjumping forces the solver to explore the whole tree. We could probably do something like count the number of nodes that the solver has searched and assert that it is high enough. Then we would know when we needed to make the tests more difficult. I think that should go in another PR, though. |
The idea isn't mine, but yes, I think limiting the stack to a reasonable, but not overly small size for all tests is a good idea to detect space leaks early on. The only drawback is that when some test fails because of a newly introduced space leak, it may not be immediately obvious.
Agreed :-) |
|
Thanks! |
The new test suite uses the solver DSL and is built with
-with-rtsopts=-M4M. I think this is a better way to check for space leaks than using the cabal executable, because it isolates the solver and uses simpler packages. Here's an example where I introduced a space leak by removing the bang patterns on the Int parameter incabal/cabal-install/Distribution/Solver/Modular/Message.hs
Line 54 in e496b09
Solver DSL (There are three tests, because I used PR #4110.)
cabal executable
I also had to fix two space leaks to make the tests pass. One occurs when the solver always has exactly one choice for the next goal. The other is caused by a bug in GHC 7.6.3.