Skip to content

Solver: Fix space leak in 'addlinking' (issue #2899). #4110

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

Merged
merged 3 commits into from
Dec 1, 2016

Conversation

grayjay
Copy link
Collaborator

@grayjay grayjay commented Nov 14, 2016

The solver creates linked nodes by copying existing unlinked nodes. Previously,
the subtrees shared data, which caused a space leak. This commit combines the
"build" and "addLinking" tree traversals so that the nodes are copied before
they are expanded into full trees.

This should prevent sharing more reliably than my previous PR, #3530.

I'd like to add a regression test, but I'm not sure how to do it without significantly lengthening the build. I wrote a test with the solver DSL (grayjay@1f1b95d) that fails when I limit the heap size. Do you think we should add another test suite that we run with a limited heap? I can't add the test to the existing unit test suite, because some of the quickcheck tests already cause spikes in memory usage.

EDIT: The diff for the main commit isn't aligned well, so here is a diff with different options: https://rawgit.com/grayjay/9d842a8c8c1b04b1a1f0d8eca4a8a2ad/raw/467ffdf7b523272a5a7118f940af9e7f2634a0eb/cabal-pr-4110.html

@mention-bot
Copy link

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

@23Skidoo
Copy link
Member

Do you think we should add another test suite that we run with a limited heap?

This is a good idea IMO.

@ezyang
Copy link
Contributor

ezyang commented Nov 18, 2016

If you want to merge this without the test I'm OK with that.

@grayjay
Copy link
Collaborator Author

grayjay commented Nov 20, 2016

@ezyang Thanks. I thought it would be easier to develop the test while the fix is still easy to revert, though.

I rebased on top of #4122, so the first four commits are from that PR.

The solver creates linked nodes by copying existing unlinked nodes. Previously,
the subtrees shared data, which caused a space leak. This commit combines the
"build" and "addLinking" tree traversals so that the nodes are copied before
they are expanded into full trees.
@grayjay
Copy link
Collaborator Author

grayjay commented Nov 22, 2016

Rebased onto master

@23Skidoo
Copy link
Member

23Skidoo commented Dec 1, 2016

I think that this should also go in.

@23Skidoo 23Skidoo merged commit 93d8e10 into haskell:master Dec 1, 2016
@23Skidoo
Copy link
Member

23Skidoo commented Dec 1, 2016

Merged, thanks!

@grayjay
Copy link
Collaborator Author

grayjay commented Dec 1, 2016

Thanks!

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.

4 participants