-
-
Notifications
You must be signed in to change notification settings - Fork 114
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)!: Custom Grain object files #2104
base: main
Are you sure you want to change the base?
Conversation
@@ -377,6 +377,6 @@ describe("basic functionality", ({test, testSkip}) => { | |||
~config_fn=smallestFileConfig, | |||
"smallest_grain_program", | |||
"", | |||
4769, | |||
6424, |
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.
Why did this go up?
Is it because of names changing and debug info?
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 meant to leave a comment about this; thanks for pointing it out. The old linker had a concept of hard/soft dependencies. If a file was a dependency but no values/functions from that module were actually uses (just types or an unused import), then the linker would compile in type information but not link in the actual files.
At first thought this seems like good behavior, but if this is done, side effects from importing those modules don't happen, which is bad. For example, a normal Grain program that doesn't use anything in Pervasives
doesn't link in Pervasives
, which means the setupExceptions
side effects don't happen, and throwing exceptions in those programs prints GrainError
instead of the actual error. In practice, users don't run into this because most programs use something from Pervasives.
This new linker links in all dependencies, which is more correct. As I mentioned, in practice this doesn't make a real difference for most users. Side note: the major reason the original implementation did this was because the compiler wasn't great at removing unused code from the resulting binary and this resulted in large module sizes. The compiler has gotten much better and this isn't really an issue anymore.
I also discovered a few more configurations producing smaller programs while working on this, as small as 25 bytes.
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.
Ah okay then this probably closes #1903 then.
c328d9c
to
92e6423
Compare
92e6423
to
89fcb11
Compare
) | ||
| None => () | ||
}, | ||
Ident_tbl.add( |
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.
Could you clarify why this was changed
@@ -548,10 +556,14 @@ type mash_program = { | |||
exports: list(export), | |||
main_body: block, | |||
main_body_stack_size: stack_size, | |||
globals: list((Ident.t, Types.allocation_type)), | |||
globals: list((Ident.t, bool, Types.allocation_type, option(constant))), |
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.
Can we make this a record now? It's not immediately clear what each of these are
if (read_version != Config.version) { | ||
raise(Error(Wrong_version_interface(filename, read_version))); | ||
}; | ||
let _ = input_binary_int(ic); |
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.
Why not do the same validation as lines 48-51 of emitmod where you verify the length is correct?
Ident.unique_name(name); | ||
}; | ||
|
||
let reset = () => { |
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.
What is the point of this function
Overview
This PR replaces Grain's intermediate
.gr.wasm
files with Grain object files. This is for a variety of reasons, including:Technical details
Grain object files use the
.gro
extension. As no wasm files are specific to Grain, the default output filename from the compiler uses the.wasm
extension rather than.gr.wasm
.Object files follow this layout:
mash_program
does include the module's cmi, but this format allows typechecking to unmarshal just the type information.We use OCaml's
Marshal
module as our binary format (and for Whole Grain, we can use ourMarshal
module). js_of_ocaml can't marshal floats into a format that can safely be read back by native code. I wanted object files to be the same between native and js, so we store floats asint64
s in the mashtree to guarantee consistent behavior.Reviewing this PR
The code delta for this PR is fairly small, but snapshot tests are now sexprs of mashtrees. As such, we have all new snapshots. I'd recommend clicking the first commit to review the code diff, or using the GitHub UI to hide changes to snapshots.
Closes #1903