Do not memoize free variables #2886
Draft
Conversation
This file contains 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.
A look at a memory profile suggested that
Syntax.mkwas allocating much of the memory to. One particular overhead it has is creating a fresh memo reference for the free variables of the term, which are memoized at every node. This change removes that memoization, which does result in less memory being used. It also seems to be faster, see the graphs below:This graph was obtained by setting OCaml's
Gc.max_overheadto zero to trigger frequent heap compactions. With this setting it's not clear that the speed improvement would carry over to a normal setting, as less allocations means less GC and therefore a lot less compactions, which are expensive. But I re-ran the test without that setting, and it seems similar: