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

Fix space leaks in dependency solver logging. #2914

Merged
merged 1 commit into from Mar 4, 2016

Conversation

Projects
None yet
4 participants
@grayjay
Copy link
Collaborator

grayjay commented Nov 6, 2015

This commit removes references to the solver log that prevented it from being garbage collected. It also forces evaluation of the current level and variable stack in Message.showMessages.

There is still a space leak in backjumping, so I profiled without backjumping enabled by merging in this branch: https://github.com/grayjay/cabal/tree/no-backjumping . I stopped each of these runs after a minute, but the times on my branch and master were similar when they solved for fewer packages.

EDIT: I compiled with GHC 7.10.2 and ran cabal --ignore-sandbox install --dry-run --max-backjumps -1 hackage-server-0.4 aeson yesod --with-compiler C:/ghc-7.6.3_64/bin/ghc +RTS -p -s -h -L50

master (bbc3638):
cabal-master

this branch (90c7772):
cabal-refactored-logging

this branch without backjumping:
cabal-refactored-logging-no-backjumping

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Nov 6, 2015

Wow. Thank you! This looks extremely promising.

You did not (intend to) change any of the actual behavior, right?

Do you think there's any way we could add some kind of regression test for this? Or even a semi-manual test? But at least something that makes it easy to test after further solver changes that the space behavior has not become worse again?

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented Nov 6, 2015

@kosmikus Thanks! My goal was to keep the existing behavior but avoid converting the Progress to and from a list of messages paired with a result.

I'm not familiar with regression testing for performance, especially comparing across versions. I mainly tested this change by checking that the heap profile leveled off after cabal finished reading the package index. A test program could at least isolate the solver and use a very small set of unchanging packages. Then a space leak would have a much larger effect on maximum residency, and we might be able to use a fixed cutoff. We could choose a difficult test and run it for a limited number of steps, so that run time isn't affected by other changes to the solver, like goal order. I'm not sure how to test for increases in constant memory use, though.

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented Nov 11, 2015

I started writing a benchmark, and it seems to detect these space leaks reliably. It runs cabal on a .cabal file with a lot of flags but no actual dependencies. The cabal.config file points to an empty package index to avoid reading the index from Hackage into memory. Then the test checks the memory use after a fixed number of backjumps with +RTS -t --machine-readable. I had to use the average, instead of the maximum, because there was often a spike at the beginning of the run.
https://github.com/grayjay/cabal/tree/solver-benchmarks .

@grayjay grayjay force-pushed the grayjay:solver-log-space-leaks branch 2 times, most recently from 5dfae2e to 3f199cb Dec 18, 2015

Fix space leaks in dependency solver logging.
This commit removes references to the solver log that prevented it from being
garbage collected.  It also forces evaluation of the current level and variable
stack in 'Message.showMessages'.

@grayjay grayjay force-pushed the grayjay:solver-log-space-leaks branch from 3f199cb to 37f28f2 Jan 17, 2016

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Jan 29, 2016

I looked through this a few days ago and it seemed reasonable to me. That said... a lot of the code complexity/churn here seems to arise from not using a special-purpose monad for the solver (and/or logging its doings) -- of course doing that would be a huge refactoring, but would it make sense?

Regardless, I don't think that this should have to wait for a method-of-doing-automated-performance-regression-testing. You've provided ample evidence that this concrete PR fixes a leak. (Don't get me wrong... if you feel like pursuing the automated-performance-regression-testing thing, then I'd be very excited about that!)

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented Jan 29, 2016

@BardurArantsson Thanks for the feedback. I'm not sure I understand the monad approach. The solver creates the log when it explores the tree, so it's only logging the last step. Earlier steps just leave information in the tree, such as pruned nodes, so that it can be included in the log. Then there are several log-processing steps. I think it would be nice to move some of that processing into the exploration step, where it might be easier. It would help with #2917 (2nd bullet point in #2917 (comment)). Also, we could remove the variable stack here if we could record the current variable during exploration:

go :: [Var QPN] -> Int -> [Message] -> [String]
.

Even though I tested this PR, I'd really like to add some solver performance tests. It's too easy to accidentally introduce performance problems. I have a test for space leaks, but I'm not sure how it could fit into the project. Testing speed would be even more useful. Do you have any ideas for testing?

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Jan 29, 2016

@grayjay It really was just an off-hand "wouldn't this be nice?" comment, so REALLY don't worry about it :).

AFAIU the PR is absolutely fine. (But I stress that my understanding of the solver is limited... very limited. Just FTR)

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented Jan 29, 2016

I was just curious because I had some ideas for refactoring that area of code, and I used a State monad in #2917. It was mostly for supporting the new feature, though.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Feb 4, 2016

@23Skidoo What do you think about just merging this given the arguments above? I think adding performance regression tests would be a different PR.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 4, 2016

@BardurArantsson
Yes, I want to see this and #2916 in 1.24, I just need to do a review first.

I think adding performance regression tests would be a different PR.

This is fine with me.

@23Skidoo 23Skidoo added this to the cabal-install 1.24.1 milestone Feb 20, 2016

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 20, 2016

Since this PR only touches cabal-install code, I'm postponing it for the 1.24.1 release.

@23Skidoo 23Skidoo modified the milestones: cabal-install 1.24.1, cabal-install 1.24 Feb 23, 2016

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 3, 2016

I'm looking at this (and #2916) right now.

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 3, 2016

Still looking. Have been running a few performance comparisons, and they all take a long time to complete. But in general, it is all looking very good so far.

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 4, 2016

This one definitely looks good to me. Still looking at #2916.

kosmikus added a commit that referenced this pull request Mar 4, 2016

Merge pull request #2914 from grayjay/solver-log-space-leaks
Fix space leaks in dependency solver logging.

@kosmikus kosmikus merged commit 83425ec into haskell:master Mar 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 4, 2016

Excellent to have this merged! I may revisit the solver split this weekend (but in a slightly more step-wise way)... @kosmikus Are you planning any big changes to the solver soon?

My revised approach will be to start by generalizing various types to have the package location be a type parameter. Unless I've missed some drastic recent developments this will allow breaking the dependency from the modular solver to cabal-install... which will be the second step.

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 4, 2016

Yes, @grayjay, let me say again that I am very very sorry for not dealing with these PRs for so long. These PRs are fantastic work. Thank you very much.

@BardurArantsson I am not generally opposed to the solver split (if we can work out the details). It's a bit difficult to say what I'm planning. It's the first time in ages I've managed to set aside a bit of time for work on this, and I will first try to look at outstanding PRs and issues. There are a few things I'd like to do myself, ultimately, but I don't think any of that will happen too soon. In any case, I'm aware of the ideas to separate out the solver, and I won't do anything major that isn't yet on the issue tracker in some form without coordinating with you.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 4, 2016

Ok, thanks. That sounds great.

I'm very happy to discuss the details -- I think the first step should be pretty uncontroversial (and non-disruptive unlike my first attempt), so I'll try to get a PR done sometime this weekend and we can perhaps discuss it then.

EDIT: I'll make sure to /cc you on that.

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 4, 2016

@BardurArantsson Ok, sounds good. Thanks.

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented Mar 4, 2016

@kosmikus No problem. Thank you for the thorough testing and review!

@grayjay grayjay deleted the grayjay:solver-log-space-leaks branch Mar 4, 2016

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 4, 2016

Thanks, @grayjay!

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 5, 2016

@kosmikus First step filed as PR #3210

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented Mar 5, 2016

I created an issue for the performance tests: #3211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment