Increase sharing in CAFs#193
Merged
k0ral merged 1 commit intosnoyberg:masterfrom Mar 19, 2024
mniip:caf
Merged
Conversation
Collaborator
|
Interesting, thanks for the fix that also happens to be a welcome simplification of the code 🙂 . |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently it's really easy to reproduce a memory "leak" with
xml-conduitwhere a part of thePipebecomes a CAF and gets evaluated arbitrarily deeply as the parser runs, while simultaneously being retained at the root because it's a CAF. Exactly how this works is explained at https://www.well-typed.com/blog/2016/09/sharing-conduit/ .The shotgun approach to this (mentioned in the blog-post) is to disable full laziness and I see a lot of people doing this (c.f. #142 and snoyberg/conduit#370)
However I decided to look deeper into it: the more nuanced advice that the article gives is that conduit pieces that do not depend on any parameters are specifically at risk of becoming CAFs. From profiling with
-hdand reading the Core I found out that this is exactly what happens toaddBeginEnd.I wasn't able to reliably prevent it from becoming a CAF (the article advises separating it into a module compiled separately under
-fno-full-laziness), however I was able to increase sharing so that the CAF loops back on itself and occupies constant space. This also fixes the memory leak and is also given as an advice in the article.The culprit seems to be replacing a fixpoint on
ConduitTwith a fixpoint onPipe, which is howawaitForeveris implemented.I don't claim to understand this issue fully, but this fixes the leaks in my use-cases. I believe this also addresses #136