-
-
Notifications
You must be signed in to change notification settings - Fork 113
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)!: Selectively include functions in the global function table #1183
Conversation
a4b6d47
to
0c03f16
Compare
20c9221
to
6159ee4
Compare
0c03f16
to
8902675
Compare
6159ee4
to
7dfc921
Compare
8902675
to
94c8d07
Compare
5907c64
to
48f3b98
Compare
94c8d07
to
ff243e2
Compare
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 is awesome! A few questions but here's a preemptive approval!
ff243e2
to
0133a2d
Compare
0133a2d
to
9b0a798
Compare
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.
LGTM!
@@ -1303,6 +1351,55 @@ let transl_signature = (~functions, ~imports, signature) => { | |||
) | |||
}; | |||
} | |||
| TSigType(tid, {type_kind: TDataVariant(cds)} as td, rs) => { |
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.
Just so I understand, can you clarify what this is doing? (In GitHub is fine)
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.
Absolutely! It just exports the functions that were created for the types you defined in the module. It won't be necessary anymore when we're not creating functions to create variants.
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.
Two minor requests for comments, but looks good otherwise!
|
||
let mark_ident_seen = id => { | ||
switch (Ident_tbl.find_opt(direct_function_calls, id)) { | ||
| Some(count) => Ident_tbl.replace(direct_function_calls, id, count + 1) |
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.
I think this +1/-1 logic could use a comment (since it's not immediately obvious what this table is tracking)
let has_indirect_call = id => { | ||
switch (Ident_tbl.find_opt(direct_function_calls, id)) { | ||
| Some(count) => count != 0 | ||
| None => true |
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.
Might warrant a comment why this defaults to true
This PR changes the module contract for first-class functions. Previously, modules put all functions in the global function table to allow any function to be called indirectly, just with a reference to the closure. Now, modules that want to be able to call a function indirectly are responsible for putting the function into the function table. This allows us to finally take advantage of tree shaking—Binaryen can completely drop unused functions 🎉
There are a couple more changes necessary to see the full effect, but with just this change, "hello world" programs are 10% smaller. Some early testing on those other changes has shown close to a 40% module size reduction.
Those other changes include some refactoring of how exports are handled, and completely avoiding closure allocation when it's not needed.
Draft because we need the fix from grain-lang/binaryen.ml#149, and after that there's one failing test due to WebAssembly/binaryen#4587.