Stream: rfc-notifications

Topic: rfcs / PR #9 RFC (Wasmtime): add host functions at the `E...

RFC notifications bot (Jan 08 2021 at 02:12):

peterhuene opened PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions at the Engine-level.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions at the Engine-level, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

RFC notifications bot (Jan 08 2021 at 15:53):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 15:53):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 15:53):

alexcrichton created PR Review Comment:

Since Store is generally behind Rc and such I think we'll want to change this and remove_context to &self instead of &mut self.

RFC notifications bot (Jan 08 2021 at 15:53):

alexcrichton created PR Review Comment:

I think we may not end up needing IntoEngineFunc as a separate trait here? It's mentioned below that this trait will also be implemented for Fn(Caller, &[Val], &mut [Val]) -> Result<Trap> + static but I don't think we can do that because we can't infer the type signature from the closure. If that's the only reason, though, then this could have two methods, one taking IntoFunc and the other being the mirror of Func::new` which takes a signature and a function?

RFC notifications bot (Jan 08 2021 at 15:53):

alexcrichton created PR Review Comment:

FWIW this doesn't look like it necessarily needs to be a function onWasi, it could presumably just be store.set_context(WasiCtx::default())?

RFC notifications bot (Jan 08 2021 at 15:54):

alexcrichton edited PR Review Comment.

RFC notifications bot (Jan 08 2021 at 16:33):

sunfishcode submitted PR Review.

RFC notifications bot (Jan 08 2021 at 16:33):

sunfishcode created PR Review Comment:

In use cases for API virtualization, the store doesn't seem like the right entity to attach contextual information to. Using the store here effectively makes all instances of an API share data.

The Wasmtime ABI passes the callee vmcontext to regular wasm functions; would it make sense to use a similar mechanism for engine functions, and store the state in something like the host_state field in Instance, instead of in the store?

RFC notifications bot (Jan 08 2021 at 16:33):

sunfishcode submitted PR Review.

RFC notifications bot (Jan 08 2021 at 16:47):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 16:47):

alexcrichton created PR Review Comment:

I'm not sure that instances are the best places to store data because they're generally designed at the wasm-spec-level to get decomposed into a bag of functions/globals/etc and those get passed around individually. The "calling instance" seems like it's sometimes a little ambiguous or just generally not available.

For virtualization, though, can you detail how something like this wouldn't work well? Wasm-based virtualization wouldn't be affected at all since it's not storing data directly in the Store, and host-based virtualization presumably wouldn't be using the exact same type of context to virtualize? (and if it is it could always use a newttype)

RFC notifications bot (Jan 08 2021 at 17:21):

fitzgen submitted PR Review.

RFC notifications bot (Jan 08 2021 at 17:21):

fitzgen created PR Review Comment:

:+1: it would be really nice to avoid another IntoFunc style trait.

• They aren't great from a user point of view since they are basically "special" sealed traits and you have to figure out what shape of things are expected.
• This seems, as Alex mentions, basically identical to the existing trait. Would be great to share a single trait if possible.

RFC notifications bot (Jan 08 2021 at 17:30):

fitzgen submitted PR Review.

RFC notifications bot (Jan 08 2021 at 17:30):

fitzgen submitted PR Review.

RFC notifications bot (Jan 08 2021 at 17:30):

fitzgen created PR Review Comment:

/// This will replace any existing `WasiCtx` associated with the `Store`.
///
/// The old `WasiCtx`, if any, is returned.
pub fn set_context(store: &Store, context: WasiCtx) -> Option<WasiCtx>;

Although, will this overwrite any context that already exists on the store? Not just a WasiCtx?

This is why it would be nice if we used ephemerons for associating context instead. (Something we can always add later, if it makes sense; nothing to block this stuff on I don't think.)

RFC notifications bot (Jan 08 2021 at 18:04):

peterhuene submitted PR Review.

RFC notifications bot (Jan 08 2021 at 18:04):

peterhuene created PR Review Comment:

The existing IntoFunc method is inherently tied to Store, which is why it isn't being used in this context.

I'm not a fan of more boilerplate code, but I'm open to somehow extending IntoFunc for this use case.

Regarding Fn(Caller, &[Val], &mut [Val]), you're absolutely right that we can't infer the Wasm type of the function with that so another method will be necessary.

RFC notifications bot (Jan 08 2021 at 18:05):

peterhuene submitted PR Review.

RFC notifications bot (Jan 08 2021 at 18:05):

peterhuene created PR Review Comment:

Agreed. I'll remove the &mut.

RFC notifications bot (Jan 08 2021 at 18:07):

peterhuene submitted PR Review.

RFC notifications bot (Jan 08 2021 at 18:07):

peterhuene created PR Review Comment:

The thinking behind this is that Wasi will likely want to wrap the WasiCtx in a RefCell and it is the only one that knows exactly what it expects the context type to be in the host functions.

RFC notifications bot (Jan 08 2021 at 18:08):

peterhuene submitted PR Review.

RFC notifications bot (Jan 08 2021 at 18:08):

peterhuene created PR Review Comment:

It would just replace the WasiCtx as a map of typeid -> context value will be used ala Lucet's set_embed_ctx.

RFC notifications bot (Jan 08 2021 at 18:09):

peterhuene edited PR Review Comment.

RFC notifications bot (Jan 08 2021 at 18:09):

peterhuene updated PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions at the Engine-level.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions at the Engine-level, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

RFC notifications bot (Jan 08 2021 at 18:10):

peterhuene edited PR Review Comment.

RFC notifications bot (Jan 08 2021 at 18:25):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 18:25):

alexcrichton created PR Review Comment:

Ah right that's a good point, but I think we can add new functions as necessary to that trait since it's not intended to be implemented externally. We might have roughtly the same amount of boilerplate internally but I think we should be able to get away witth one trait.

RFC notifications bot (Jan 08 2021 at 18:26):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 18:26):

alexcrichton created PR Review Comment:

Ah ok that's a good point yeah, seems reasonable!

RFC notifications bot (Jan 08 2021 at 18:30):

peterhuene submitted PR Review.

RFC notifications bot (Jan 08 2021 at 18:30):

peterhuene created PR Review Comment:

For more context, I originally proposed (in a privately reviewed draft) that the context be set on the instance, so I'm not opposed to that.

I think I was unclear in the intention of set_context allowing multiple values of different types, which is what Lucet does today with insert_embed_ctx. As such, "insert" would probably be a better name as "set" has a singular connotation; I'll make that change.

If we do store it on the instance, we'll obviously need to modify the wasmtime_runtime::Instance to store the values and then Caller needs to be modified to perhaps return a wasmtime::Instance to get the values from?

That said, if we keep it on the Store, it's true that all instances using the Wasi host functions added at the engine-level will share the same WasiCxt context. But that still does not preclude users from being able to call Wasi::add_to_linker to link in WASI with a different context for a specific instantiation.

RFC notifications bot (Jan 08 2021 at 18:33):

peterhuene edited PR Review Comment.

RFC notifications bot (Jan 08 2021 at 19:02):

peterhuene submitted PR Review.

RFC notifications bot (Jan 08 2021 at 19:02):

peterhuene created PR Review Comment:

Agreed. I'll update the draft with extensions for IntoFunc.

RFC notifications bot (Jan 08 2021 at 19:21):

peterhuene edited PR Review Comment.

RFC notifications bot (Jan 08 2021 at 19:22):

peterhuene edited PR Review Comment.

RFC notifications bot (Jan 08 2021 at 19:39):

peterhuene updated PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions at the Engine-level.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions at the Engine-level, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

RFC notifications bot (Jan 08 2021 at 19:58):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 19:58):

alexcrichton created PR Review Comment:

Oh I also think that this is going to need Send and Sync bounds to keep Engine both Send and Sync (and that'd also highlight that as an engine-global thing the function may get invoked concurrently)

RFC notifications bot (Jan 08 2021 at 19:58):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 19:58):

alexcrichton created PR Review Comment:

Similar to above this'll probably need + Send + Sync

RFC notifications bot (Jan 08 2021 at 20:01):

peterhuene submitted PR Review.

RFC notifications bot (Jan 08 2021 at 20:01):

peterhuene created PR Review Comment:

Most definitely will. shakes fist at Markdown not compiling his code

RFC notifications bot (Jan 08 2021 at 20:07):

peterhuene updated PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions at the Engine-level.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions at the Engine-level, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

RFC notifications bot (Jan 08 2021 at 20:10):

sunfishcode submitted PR Review.

RFC notifications bot (Jan 08 2021 at 20:10):

sunfishcode created PR Review Comment:

To be sure, I mean the callee vmctx arg, and not the caller one. The caller is indeed ambiguous and will ideally go away once interface types obviates exporting "memory". The callee, however, is pretty fundamental. Wasm functions need their instances.

The use case I'm thinking of is: an API which has some state associated with it. If two parts of an application each want to use that API, independently of the other, they should be able to have independent copies of the state. Ideally, we want host APIs to act as much like wasm APIs as possible, so that we can easily switch between host and polyfill implementations.

That said, if we keep it on the Store, it's true that all instances using the Wasi host functions added at the engine-level will share the same WasiCxt context. But that still does not preclude users from being able to call Wasi::add_to_linker to link in WASI with a different context for a specific instantiation using the same store.

It's good that you can do multiple contexts; what I'm asking is, should this be the default way of doing things, and perhaps even the only way? Right now, there's not a lot attached to the store, and the state that is there is well understood, which is a useful property when we think about suspending/resuming/checkpoint-restart/migration/etc. if instances.

Here's a potentially crazy idea: each imported function has a VMCallerCheckedAnyfunc, which includes a *mut VMContext. But after instantiation and linking are done, one instantace doesn't ever look at the *mut VMContext for another instance. It just just blindly passes the context argument when calling an import. So, what if we generalized this pointer, and said it's no longer just a *mut VMContext, but an arbitrary "context" state pointer? For wasm modules, it'd still be the same VMContext pointer, but for host modules, it could be a pointer into whatever type the host wanted? Then we wouldn't even have to figure out how to fit this into Instance.

RFC notifications bot (Jan 08 2021 at 20:20):

alexcrichton submitted PR Review.

RFC notifications bot (Jan 08 2021 at 20:20):

alexcrichton created PR Review Comment:

Oh sorry I misunderstood about callee/caller. I'm curious though how you're thinking that the callee's state is initialized? For per-function state you can always simply capture state via the Rust closure, but the main problem with that is then the state is per-engine rather than per-instance/store. Where would per instance/store state get attached in what you're thinking @sunfishcode? (currently it's set_context for what this currently proposes I believe)

RFC notifications bot (Jan 09 2021 at 00:52):

sunfishcode submitted PR Review.

RFC notifications bot (Jan 09 2021 at 00:52):

sunfishcode created PR Review Comment:

Ok yeah, that is tricky. I'll have to think about this some more.

RFC notifications bot (Jan 30 2021 at 00:23):

peterhuene updated PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions at the Engine-level.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions at the Engine-level, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

RFC notifications bot (Jan 30 2021 at 00:24):

peterhuene has marked PR #9 as ready for review.

RFC notifications bot (Jan 30 2021 at 00:25):

peterhuene updated PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions at the Engine-level.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions at the Engine-level, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

RFC notifications bot (Jan 30 2021 at 00:28):

peterhuene submitted PR Review.

RFC notifications bot (Jan 30 2021 at 00:28):

peterhuene created PR Review Comment:

@sunfishcode Do you have any suggestions as to an alternative approach? I'd like to resolve this prior to motion to finalize.

RFC notifications bot (Feb 02 2021 at 23:48):

peterhuene updated PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions at the Engine-level.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions at the Engine-level, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

RFC notifications bot (Feb 02 2021 at 23:48):

peterhuene edited PR #9 from engine-host-functions to main:

Rendered RFC

This RFC proposes to extend the Wasmtime API to allow users to define host
functions in a Config that can be shared between multiple engines and
therefore stores.

Today, Func is used to define host functions, but it is tied to a Store.
If users desire short-lived stores to ensure short-lived instances, host
functions need to be redefined upon every module instantiation.

By defining host functions with a Config, instances can be created
without having to redefine the host functions; this will make for faster module
instantiations in scenarios where a module is repeatedly instantiated.

Last updated: Nov 22 2024 at 17:03 UTC