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

Deepseq the result of typechecking a module to avoid a space leak. #891

Closed
wants to merge 1 commit into from

Conversation

brianhuffman
Copy link
Contributor

Previously, unevaluated apSubst thunks were retaining copies of
a very large Subst value.

Fixes #888.

Previously, unevaluated `apSubst` thunks were retaining copies of
a very large `Subst` value.

Fixes #888.
@brianhuffman
Copy link
Contributor Author

I'm looking for some feedback here: I'm not sure that tcModule is the nicest place to put the deepseq call, as the motivation for it depends on knowing that runInferM uses apSubst internally. Maybe it would be better, and make the code more understandable and maintainable, to add deepseq calls to runInferM instead? Are there any cases where we wouldn't want to use deepseq on the result of runInferM? Or maybe deepseq is too blunt of an instrument; should we just make apSubst more strict instead?

@robdockins
Copy link
Contributor

I usually prefer to add strictness annotations to datatypes rather than using deepseq. I think it makes the programmer intent more clear, and it is less fragile against future code changes than manually inserting seq or deepseq. Deepseq also requires and additional pass over datatypes to force them, whereas constructing them more strictly in the first place avoids an extra pass. Finally, I also think it helps the compiler produce better code by propagating strictness information throughout the program; you can avoid building thunks in the first place, and save some allocations, etc.

@brianhuffman
Copy link
Contributor Author

brianhuffman commented Sep 12, 2020

I would probably prefer that too, but the typed-AST data types use recursion through Maybe, list, Map, etc; those would have to be replaced with strict variants.

I think that adding strictness to the definition of apSubst would be a good design; it already has to do a pass over the whole AST, so making it more strict wouldn't add any additional work. Also, it applies strictness in exactly the right places, since you can ignore those subterms that don't contain variables that need to be substituted.

@brianhuffman
Copy link
Contributor Author

Closing in favor of #895.

@RyanGlScott RyanGlScott deleted the issue888 branch March 22, 2024 14:45
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.

Memory leak related to type substitutions
2 participants