Skip to content
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

Refactor the internals of Func to remove layers of indirection #1363

Merged
merged 9 commits into from
Mar 19, 2020

Conversation

alexcrichton
Copy link
Member

This commit simplifies Func and its API through a few avenues:

  • The callable module is removed in favor of always calling functions through their trampoline (now that we always have one)
  • The Callable trait is removed in favor of just using an impl Fn() argument
  • The Func::new method is now generic and no longer requires an explicit Rc allocation
  • The Func::new closure now takes a leading &Caller argument to empower it with the same capabilities of Func::wrap

At this point `Func` has evolved quite a bit since inception and the
`WrappedCallable` trait I don't believe is needed any longer. This
should help clean up a few entry points by having fewer traits in play.
This commit removes the `wasmtime::Callable` trait, changing the
signature of `Func::new` to take an appropriately typed `Fn`.
Additionally the function now always takes `&Caller` like `Func::wrap`
optionally can, to empower `Func::new` to have the same capabilities of
`Func::wrap`.
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Mar 19, 2020
@github-actions
Copy link

Subscribe to Label Action

This issue or pull request has been labeled: "wasmtime:api"

Users Subscribed to "wasmtime:api"

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@github-actions github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:c-api Issues pertaining to the C API. labels Mar 19, 2020
@github-actions
Copy link

Subscribe to Label Action

This issue or pull request has been labeled: "fuzzing", "wasmtime:c-api"

Users Subscribed to "fuzzing"
Users Subscribed to "wasmtime:c-api"

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a nice simplification!

// Create our actual trampoline function which translates from a bunch
// of bit patterns on the stack to actual instances of `Val` being
// passed to the given function.
let func = Box::new(move |caller_vmctx, values_vec: *mut i128| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use u128 for values in a lot of places, Is there a reason to prefer i128 here?

Random idea, don't feel obliged to do this here, but we could also introduce a new type, like VMValue or so, which has the size and alignment of a u128, that we could use instead of u128 everywhere, to help document the intent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah this was just carried over from the original implementation, I'll switch it all to u128

crates/api/src/func.rs Outdated Show resolved Hide resolved
crates/api/src/func.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants