Allow serializing all cranelift-module data structures #6172
Conversation
This file contains hidden or 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
bjorn3
force-pushed
the
bsc-unwinding-module-serialize2
branch
from
6a5811e to
436ffb8
Compare
bjorn3
commented
Apr 7, 2023
|
|
||
| struct ModuleDeclarationsFieldVisitor; | ||
|
|
||
| impl<'de> serde::de::Visitor<'de> for ModuleDeclarationsFieldVisitor { |
Contributor
Author
There was a problem hiding this comment.
The Deserialize impl is based on serde-derive's macro expansion, but it has been cleaned up a lot and modified to run get_names instead of expecting a names field. This should be license technically, right?
Member
There was a problem hiding this comment.
Any chance you can use one of the facilities provided by serde, e.g. #[serde(serialize_with = ...)] on the names field, instead?
Contributor
Author
There was a problem hiding this comment.
When applied to the names field that doesn't give access to the functions and data_objects fields to reconstruct the names field from from what I can tell.
github-actions
bot
added
cranelift
fitzgen
requested review from
bnjbvr
and removed request for
a team and
fitzgen
Member
|
Redirecting review to @bnjbvr who has touched these files recently (I've never messed with |
bjorn3
force-pushed
the
bsc-unwinding-module-serialize2
branch
from
436ffb8 to
0f1fbe2
Compare
bnjbvr
reviewed
Apr 11, 2023
Member
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
I really just happened to touch this file, but I'm really not a code owner of it, sorry. Just a few small comments while skimming the code.
|
|
||
| struct ModuleDeclarationsFieldVisitor; | ||
|
|
||
| impl<'de> serde::de::Visitor<'de> for ModuleDeclarationsFieldVisitor { |
Member
There was a problem hiding this comment.
Any chance you can use one of the facilities provided by serde, e.g. #[serde(serialize_with = ...)] on the names field, instead?
bjorn3
force-pushed
the
bsc-unwinding-module-serialize2
branch
from
3760b5f to
6784ad9
Compare
Contributor
Author
|
This is ready for review again. |
The same information can be retrieved using ctx.compiled_code().unwrap().code_info().total_size In addition for Module implementations that don't immediately compile the given function there is no correct value that can be returned.
This internal name can conflict if a module is serialized and then deserialized into another module. It also wasn't used by any of the Module implementations anyway.
This allows a Module implementation to serialize it's internal state and deserialize it in another compilation session. For example to implement LTO or to load the module into cranelift-interpreter.
bjorn3
force-pushed
the
bsc-unwinding-module-serialize2
branch
from
6784ad9 to
32580fb
Compare
afonso360
pushed a commit
to afonso360/wasmtime
that referenced
this pull request
Apr 24, 2023
…nce#6172) * Remove ModuleCompiledFunction The same information can be retrieved using ctx.compiled_code().unwrap().code_info().total_size In addition for Module implementations that don't immediately compile the given function there is no correct value that can be returned. * Don't give anonymous functions and data objects an internal name This internal name can conflict if a module is serialized and then deserialized into another module. It also wasn't used by any of the Module implementations anyway. * Allow serializing all cranelift-module data structures This allows a Module implementation to serialize it's internal state and deserialize it in another compilation session. For example to implement LTO or to load the module into cranelift-interpreter. * Use expect
eduardomourar
pushed a commit
to eduardomourar/wasmtime
that referenced
this pull request
Apr 28, 2023
…nce#6172) * Remove ModuleCompiledFunction The same information can be retrieved using ctx.compiled_code().unwrap().code_info().total_size In addition for Module implementations that don't immediately compile the given function there is no correct value that can be returned. * Don't give anonymous functions and data objects an internal name This internal name can conflict if a module is serialized and then deserialized into another module. It also wasn't used by any of the Module implementations anyway. * Allow serializing all cranelift-module data structures This allows a Module implementation to serialize it's internal state and deserialize it in another compilation session. For example to implement LTO or to load the module into cranelift-interpreter. * Use expect
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 allows a Module implementation to serialize it's internal state and deserialize it in another compilation session. For example to implement LTO or to load the module into cranelift-interpreter.
This is the last PR in the series for now. The actual implementation of a module that can be serialized is in cg_clif. I may extract it in the future, but for now these are almost all the changes that need to be made on the cranelift side. The previous PR's in this series are #6169 and #6170.