Refactor #1524
Merged
Refactor #1524
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
sunfishcode
added
the
wasmtime:api
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
github-actions
bot
added
fuzzing
There was a problem hiding this comment.
Looks pretty good to me! My worry about this though is that it's making a lot more parts of the API "owned" where it's no longer behind a reference. That means that some places are a little more unergonomic (e.g. getting the nth export as a function) and some may involve more clones (seems like there's lots of various clones here and there).
Overall it does seem pretty nice to avoid trying to duplicate information between the api crate and its internals though. I wonder if we could perhaps try to assuage some of this ergonomic/cloning overhead with some different idioms? We could, for example, add some apis to Instance to do something like "get the nth function" or switch all the examples to "get the export named foo".
Subscribe to Label Actioncc @fitzgen, @peterhuene
This issue or pull request has been labeled: "fuzzing", "wasmtime:c-api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
sunfishcode
force-pushed
the
refactor
branch
2 times, most recently
from
aa3589e
to
186573c
Compare
Instead having instances eagerly compute a Vec of Externs, and bumping the refcount for each Extern, compute Externs on demand. This also enables `Instance::get_export` to avoid doing a linear search. This also means that the closure returned by `get0` and friends now holds an `InstanceHandle` to dynamically hold the instance live rather than being scoped to a lifetime.
And compute Extern::ty on demand too.
This helps differentiate it from other Export types in the tree, and describes what it is.
This significantly simplifies the public API, as it's relatively common to need the names, and this avoids the need to do a zip with `Module::exports`. This also changes `ImportType` and `ExportType` to have public members instead of private members and accessors, as I find that simplifies the usage particularly in cases where there are temporary instances.
This doesn't quite remove `Instance`'s `module` member, it gets a step closer.
Instead, just create a closure containing the instance handle and the export for them to call.
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @bnjbvr
This issue or pull request has been labeled: "cranelift", "cranelift:wasm"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This makes them more consistent with ExportType.
Instead of 'me, use 'instance and 'module to make it clear what the lifetime is.
alexcrichton
approved these changes
Apr 20, 2020
CryZe
pushed a commit
to CryZe/wasmtime
that referenced
this pull request
Apr 25, 2020
* Compute instance exports on demand. Instead having instances eagerly compute a Vec of Externs, and bumping the refcount for each Extern, compute Externs on demand. This also enables `Instance::get_export` to avoid doing a linear search. This also means that the closure returned by `get0` and friends now holds an `InstanceHandle` to dynamically hold the instance live rather than being scoped to a lifetime. * Compute module imports and exports on demand too. And compute Extern::ty on demand too. * Add a utility function for computing an ExternType. * Add a utility function for looking up a function's signature. * Add a utility function for computing the ValType of a Global. * Rename wasmtime_environ::Export to EntityIndex. This helps differentiate it from other Export types in the tree, and describes what it is. * Fix a typo in a comment. * Simplify module imports and exports. * Make `Instance::exports` return the export names. This significantly simplifies the public API, as it's relatively common to need the names, and this avoids the need to do a zip with `Module::exports`. This also changes `ImportType` and `ExportType` to have public members instead of private members and accessors, as I find that simplifies the usage particularly in cases where there are temporary instances. * Remove `Instance::module`. This doesn't quite remove `Instance`'s `module` member, it gets a step closer. * Use a InstanceHandle utility function. * Don't consume self in the `Func::get*` methods. Instead, just create a closure containing the instance handle and the export for them to call. * Use `ExactSizeIterator` to avoid needing separate `num_*` methods. * Rename `Extern::func()` etc. to `into_func()` etc. * Revise examples to avoid using `nth`. * Add convenience methods to instance for getting specific extern types. * Use the convenience functions in more tests and examples. * Avoid cloning strings for `ImportType` and `ExportType`. * Remove more obviated clone() calls. * Simplify `Func`'s closure state. * Make wasmtime::Export's fields private. This makes them more consistent with ExportType. * Fix compilation error. * Make a lifetime parameter explicit, and use better lifetime names. Instead of 'me, use 'instance and 'module to make it clear what the lifetime is. * More lifetime cleanups.
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 contains refactorings and API changes split off from #1516.
The main public-facing change here is that
InstanceandModulenow compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*return closures which capture anInstanceHandleso that they hold the instance live dynamically.