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 leak in solver backjumping #2916

Merged
merged 2 commits into from Mar 4, 2016

Conversation

Projects
None yet
5 participants
@grayjay
Copy link
Collaborator

grayjay commented Nov 8, 2015

This commit refactors backjumping so that it uses the Progress type instead of
separate references to a node's children and the conflict set calculated from
those children. The behavior should be unchanged.

I added to #2914 so that I could test memory usage more easily. These heap profiles were generated with 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.

after #2914 (90c7772):
cabal-refactored-logging

after both commits (c1189c3):
cabal

longer run with both commits, using -i10. I don't know why it increases later.
cabal-longer-run

I have a few concerns about this PR. I combined backjumping and exploring the tree into one traversal, which is less modular. Is there a way that we could use two passes without storing conflict sets directly on the tree's nodes? The combine function is also less recognizable after the refactoring. I'd like to make it clearer, if possible.

I deleted several unused functions that didn't compile with my change, such as explore. Is that alright, or should I refactor them?

@grayjay grayjay force-pushed the grayjay:backjumping-space-leak branch from e3cab66 to c1189c3 Nov 8, 2015

@@ -135,15 +80,12 @@ exploreLog = cata go
-- tree, but rather make assumptions about where that shape originated from. It'd be
-- better if the pruning itself would leave information that we could pick up at this
-- point.

This comment has been minimized.

@grayjay

grayjay Nov 8, 2015

Collaborator

I'm not sure if this comment still applies to this function after I changed it.

@grayjay grayjay force-pushed the grayjay:backjumping-space-leak branch 3 times, most recently from aa8a029 to e742305 Dec 18, 2015

grayjay added some commits Nov 6, 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'.
Fix space leak in solver backjumping
This commit refactors backjumping so that it uses the 'Progress' type instead of
separate references to a node's children and the conflict set calculated from
those children.

@grayjay grayjay force-pushed the grayjay:backjumping-space-leak branch from e742305 to 8923a46 Jan 17, 2016

@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 4, 2016

@grayjay So far, this is looking good as well. I can confirm/reproduce your profiling results, and performance tests seem to be ok.

Also, I don't think merging explore and backjump into a single function is all that tragic. The interactions were very subtle before as well, so they weren't entirely modular anyway. I still want to look a bit more at the actual new code though.

@kosmikus

This comment has been minimized.

Is it even worth keeping this as a function? I think it might be easier to inline in the two(?) places it is called?

This comment has been minimized.

Copy link
Owner

grayjay replied Mar 4, 2016

I only kept it because I wasn't sure what to do with the comment. Does it still apply?

This comment has been minimized.

Copy link

kosmikus replied Mar 4, 2016

I think we can remove it.

This comment has been minimized.

Copy link
Owner

grayjay replied Mar 4, 2016

Okay. I'll delete the comment and move the function into backjump.

This comment has been minimized.

Copy link
Owner

grayjay replied Mar 4, 2016

I made a PR: haskell#3209

@kosmikus

This comment has been minimized.

Sorry about the comment I made here before. I did not read the call to foldr correctly.

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 4, 2016

Ok, I'm happy with the code and going to merge this. Quite drastic space wins for more complicated solver tasks on this one, and at least near-constant space behavior of the solver for really long runs now. Really great work. Thanks again.

@kosmikus kosmikus merged commit 8923a46 into haskell:master Mar 4, 2016

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

grayjay commented Mar 4, 2016

@kosmikus Thank you!

@grayjay grayjay deleted the grayjay:backjumping-space-leak branch Mar 4, 2016

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 4, 2016

Cherry-picked both this and #2914 into 1.24.

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented Mar 4, 2016

Thanks!

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 4, 2016

@23Skidoo Does this mean that master is once again open for big changes? (Sorry, I haven't had time to follow along too closely with Cabal goings-on.)

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Mar 4, 2016

@BardurArantsson In principle, yes, but I've been focusing on PRs that have a chance of being included in 1.24 (given that we have some time until the GHC 8 release). So the 1.24 branch hasn't really diverged from master yet.

Right now, the remaining ones are (ordered by priority):

  • #2540 - platform libraries
  • #2522 - globstar globbing
  • #2774 - gen-bounds command
  • #2882 - status command
  • #3202 - cabal list --dependencies
@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 4, 2016

Ah, right, so ideally things that cause less distruption should be merged first? I don't think my reworked solver-split changes will necessarily need to be merged to master immediately (but OTOH they should be obvious enough to not cause any breakage). It's just one of those things that probably will cause conflicts for any other solver changes. Anywho, I'll give it a go and we'll see what the damage is :)

@hvr

This comment has been minimized.

Copy link
Member

hvr commented Mar 5, 2016

@BardurArantsson fyi, @kosmikus is currently looking at solver issues & PRs (and his time is sadly rather limited)... and right now it's still easy to cherry-pick stuff he lands to master to 1.24...

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented Mar 5, 2016

@hvr See the discussion in #2914 for additional discussion. Do you mean to imply that we should even consider the split for 1.24 if it is ready soon enough?

@hvr

This comment has been minimized.

Copy link
Member

hvr commented Mar 5, 2016

@kosmikus tbh, I don't think it's essential getting this refactoring into the 1.24 branch; we're still in the process of merging the nix-local-build branch into 1.24 and it'd be helpful if master and 1.24 wouldn't diverge too much as this will only cause more work otherwise, current delta (between the nix branch and 1.24):

 .travis.yml                                               |   31 ++-
 Cabal/Cabal.cabal                                         |    2 +-
 Cabal/Distribution/Simple/PackageIndex.hs                 |    2 +-
 Cabal/Distribution/Simple/Setup.hs                        |    1 +
 cabal-install/Distribution/Client/BuildTarget.hs          | 1596 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 cabal-install/Distribution/Client/Dependency/Types.hs     |    5 +
 cabal-install/Distribution/Client/DistDirLayout.hs        |  131 ++++++++++
 cabal-install/Distribution/Client/FileMonitor.hs          |    9 +
 cabal-install/Distribution/Client/Glob.hs                 |   12 +-
 cabal-install/Distribution/Client/GlobalFlags.hs          |   48 ++--
 cabal-install/Distribution/Client/InstallPlan.hs          |  108 +++++++-
 cabal-install/Distribution/Client/MultiPkg.hs             |  745 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 cabal-install/Distribution/Client/PackageHash.hs          |  224 +++++++++++++++++
 cabal-install/Distribution/Client/ParseUtils.hs           |  240 +++++++++++++++++-
 cabal-install/Distribution/Client/ProjectBuilding.hs      | 1255 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 cabal-install/Distribution/Client/ProjectConfig.hs        |  679 ++++++++++++++++++++++++++++++++++++++++++++++++++
 cabal-install/Distribution/Client/ProjectConfig/Legacy.hs | 1077 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 cabal-install/Distribution/Client/ProjectConfig/Types.hs  |  333 +++++++++++++++++++++++++
 cabal-install/Distribution/Client/ProjectPlanning.hs      | 2328 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 cabal-install/Distribution/Client/RebuildMonad.hs         |   15 +-
 cabal-install/Distribution/Client/Setup.hs                |    1 +
 cabal-install/Distribution/Client/Types.hs                |   11 +
 cabal-install/Distribution/Client/Utils/Json.hs           |  225 +++++++++++++++++
 cabal-install/Main.hs                                     |   49 +++-
 cabal-install/cabal-install.cabal                         |   24 +-
 cabal.project                                             |    1 +
 26 files changed, 9095 insertions(+), 57 deletions(-)

...that being said, since as far as I understand it, the solver split won't touch the Cabal library at all (since only cabal-install would depend on the new split-off solver library package), this could in theory be backported/cherry-picked to the 1.24 branch lateron if there's any compelling reason to do so.

Morever, there may very well be an interim 1.26 branch not bundled with any GHC release (like there was the 1.20 branch inbetween GHC 7.8 and GHC 7.10), before a 1.28 branch to go with GHC 8.2

So there's imho no hurry as there's plenty of opportunities to release such a solver package into the open in the forseeable future. And there's still the question of how to version the solver library and how to reconcile this with Git... for instance, having all in a single Git repo will severly limit our options, and effectively force use to develop the solver library in lock-step with cabal-install and resulting in an even more intertwined and confusing history (IMO, Git handles multiple-deliverables-in-a-single-Git-repo very poorly -- having Cabal and cabal-install in the same Git repo already results in confusing Git tagging scenarios and makes git merge effectively unusable, adding yet another versioned item will make this only worse)

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 5, 2016

The new(TM) solver-split thing doesn't touch Cabal at all (nor did the old one, but...). The first step is simply about adding a type parameter to make it possible to split cabal-install and the modular solver. I'll try to submit a PR later today so we have something concrete to discuss :).

EDIT: Sorry, ninja edit:

And there's still the question of how to version the solver library and how to reconcile this with Git... for instance, having all in a single Git repo will severly limit our options, and effectively force use to develop the solver library in lock-step with cabal-install and resulting in an even more intertwined and confusing history (IMO, Git handles multiple-deliverables-in-a-single-Git-repo very poorly -- having Cabal and cabal-install in the same Git repo already results in confusing Git tagging scenarios, adding yet another versioned item will make this only worse)

The idea is to have cabal-solver to be a dependency between Cabal and cabal-install. (At least as a first step. We could consider making it completely independent of Cabal, but I'm not sure there's much value in that.)

@hvr

This comment has been minimized.

Copy link
Member

hvr commented Mar 5, 2016

@BardurArantsson that, indeed, sounds rather harmless... looking forward to the patch

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented Mar 5, 2016

@hvr First step filed as PR #3210

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