-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
feat(compiler)!: Allocate closures only when necessary #1660
Conversation
assert ImmutablePriorityQueue.make(compare) == ImmutablePriorityQueue.empty | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compare
in one module is no longer necessarily equal to compare
in another module because they can be different closures. There's no real equivalent of this test now. ==
currently just compares pointers for closures.
A test that checks something similar to this would be this: assert ImmutablePriorityQueue.isEmpty(ImmutablePriorityQueue.make(compare))
, but I do not believe that was the essence of this test.
@@ -81,7 +81,7 @@ describe("garbage collection", ({test, testSkip}) => { | |||
); | |||
assertRunGCError( | |||
"fib_gc_err", | |||
1024, | |||
512, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer ran out of memory at 1024 bytes because no closure is allocated when fib
is called anymore. I adjusted it down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about the snapshots (are they becoming tests that are completely optimized away?) otherwise looks good to me. We gave @peblair and the rest of the team a lot of time to review.
) | ||
(local.get $0) | ||
) | ||
(i32.const 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now one of those tests where we are optimizing the whole thing away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole closure gets optimized away here (which shows that the feature is working!), but not the rest of the test.
9f4c8d9
to
aa2e718
Compare
* feat(compiler)!: Allocate closures only when necessary * snapshots
Closes #1410
Works towards #1029
This PR only allocates closures when absolutely necessary—when a function actually closes over values or the function may be used in a first-class way.
This results in a 40% module size reduction for hello world programs, and similar 20-40% module size reductions for most Grain programs. It also improves program performance and module startup time.
This is breaking at the CMI level because consuming modules may need to allocate a closure for an imported function if it plans to use it in a first-class way.