feat(compiler)!: Selectively include functions in the global function table #1183
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
ospencer
force-pushed
the
oscar/func-table-refactor
branch
from
a4b6d47
to
0c03f16
Compare
ospencer
force-pushed
the
oscar/function-reexport
branch
from
20c9221
to
6159ee4
Compare
ospencer
force-pushed
the
oscar/func-table-refactor
branch
from
0c03f16
to
8902675
Compare
ospencer
force-pushed
the
oscar/function-reexport
branch
from
6159ee4
to
7dfc921
Compare
ospencer
force-pushed
the
oscar/func-table-refactor
branch
from
8902675
to
94c8d07
Compare
phated
force-pushed
the
phated/binaryen-107
branch
from
5907c64
to
48f3b98
Compare
ospencer
force-pushed
the
oscar/func-table-refactor
branch
from
94c8d07
to
ff243e2
Compare
phated
approved these changes
May 12, 2022
ospencer
force-pushed
the
oscar/func-table-refactor
branch
from
ff243e2
to
0133a2d
Compare
ospencer
force-pushed
the
oscar/func-table-refactor
branch
from
0133a2d
to
9b0a798
Compare
jozanza
approved these changes
May 15, 2022
peblair
reviewed
May 15, 2022
| @@ -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.
Just so I understand, can you clarify what this is doing? (In GitHub is fine)
There was a problem hiding this comment.
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.
peblair
requested changes
May 15, 2022
|
|
||
| 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.
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.
Might warrant a comment why this defaults to true
peblair
approved these changes
May 15, 2022
Merged
This was referenced Mar 20, 2023
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.
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.