Skip to content

Don't copy when resolving aliases in try_modtypes #143

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 1 commit into from
Mar 9, 2023

Conversation

rleshchinskiy
Copy link
Contributor

We were calling expand_module_alias to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This substantially reduces the peak heap size (and improves compile times, albeit to a lesser degree) in some cases.

Note that #119 changes this area significantly so I didn't want to refactor too much here.

We were calling `expand_module_alias` to get the unstrengthened,
non-lazy type (potential deep copy), then checking whether it's an ident
and then strengthening it, thus throwing most of it away. This patch
avoids doing that by working directly on the lazy representation. This
substantially reduces the peak heap size (and improves compile times,
albeit to a lesser degree) in some cases.

Note that ocaml-flambda#119 changes
this area significantly so I didn't want to refactor too much here.
@rleshchinskiy rleshchinskiy requested a review from lpw25 March 9, 2023 13:20
Copy link
Contributor

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lpw25 lpw25 merged commit 3f9bd64 into ocaml-flambda:main Mar 9, 2023
@rleshchinskiy rleshchinskiy deleted the expand-alias branch March 9, 2023 14:47
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