Skip to content
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

Increase sharing in CAFs #193

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Increase sharing in CAFs #193

merged 1 commit into from
Mar 19, 2024

Conversation

mniip
Copy link
Contributor

@mniip mniip commented Mar 15, 2024

Currently it's really easy to reproduce a memory "leak" with xml-conduit where a part of the Pipe becomes 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 -hd and reading the Core I found out that this is exactly what happens to addBeginEnd.

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 ConduitT with a fixpoint on Pipe, which is how awaitForever is 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

@k0ral
Copy link
Collaborator

k0ral commented Mar 19, 2024

Interesting, thanks for the fix that also happens to be a welcome simplification of the code 🙂 .

@k0ral k0ral merged commit 300e25e into snoyberg:master Mar 19, 2024
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants