Implement shared host functions. #2625

peterhuene · 2021-01-30T02:09:23Z

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with a Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

github-actions · 2021-01-30T03:06:22Z

Subscribe to Label Action

cc @kubkon, @peterhuene

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

Thus the following users have been cc'd because of the following labels:

kubkon: wasi
peterhuene: wasmtime:api

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

Learn more.

crates/wasmtime/src/engine.rs

crates/wasmtime/src/trampoline/func.rs

crates/wasmtime/src/engine.rs

crates/wasmtime/src/store.rs

crates/wasmtime/src/engine.rs

crates/wasmtime/src/linker.rs

crates/wasmtime/src/store.rs

crates/wasmtime/src/func.rs

crates/wasmtime/src/engine.rs

fitzgen

(Just dropping my thoughts; leaving proper review and signoff to Alex, therefore I'm "commenting" rather than "approving".)

crates/wasmtime/src/config.rs

crates/wasmtime/src/store.rs

peterhuene · 2021-03-08T21:55:29Z

Now that both the async PR and wiggle support for async have landed, I need to add support for async shared host functions too.

I'll push that up with a rebase.

peterhuene · 2021-03-09T03:02:18Z

This isn't quite done as I need to add tests for Config::wrap_host_func$N_async to the shared host function tests (it's minimally tested with the update to the wasmtime wiggle tests, but more tests are needed).

github-actions · 2021-03-09T03:45:29Z

Subscribe to Label Action

cc @peterhuene

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

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:c-api

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

Learn more.

alexcrichton · 2021-03-09T15:12:57Z

Looking good to me!

Could you detail more why the removal of the lifetime from Caller is necessary? That's currently done for safety to ensure you don't persist the structure outside the callback (since it's got a raw pointer only valid for the lifetime of the callback). It sounds and/or looks like it's due to async lifetime issues, but do you have an example of where things went wrong?

peterhuene · 2021-03-09T19:35:20Z

I did run afoul of the borrow-checker when implementing the wiggle wasmtime integration as the context is coming from Caller via the store, although with the store cloned I can't say why the lifetime was not long enough.

Let me double check that this is absolutely necessary before we proceed with this, as it is a breaking change for API users.

peterhuene · 2021-03-09T20:47:27Z

It looks like I was missing a lifetime constraint on the future in the callback signature, so I'll revert the change to Caller.

This commit introduces defining host functions at the `Config` rather than with `Func` tied to a `Store`. The intention here is to enable a host to define all of the functions once with a `Config` and then use a `Linker` (or directly with `Store::get_host_func`) to use the functions when instantiating a module. This should help improve the performance of use cases where a `Store` is short-lived and redefining the functions at every module instantiation is a noticeable performance hit. This commit adds `add_to_config` to the code generation for Wasmtime's `Wasi` type. The new method adds the WASI functions to the given config as host functions. This commit adds context functions to `Store`: `get` to get a context of a particular type and `set` to set the context on the store. For safety, `set` cannot replace an existing context value of the same type. `Wasi::set_context` was added to set the WASI context for a `Store` when using `Wasi::add_to_config`.

alexcrichton

Ok everything looks good to me modulo one bit. Currently we have Func::new_async panic if it's not paired with an async store, and similarly Func::new panics if paired with an async store. We can't panic when Config::* functions are called to define function since we don't have a store, but I think today you can define an async host function which gets paired with a sync store, which I don't think should work.

I think we could solve this one of a few ways:

Move the async-ness to Config instead of Store, panic in Config on a mismatch
Return None from get_host_func if the async-ness of the function mismatches with the store.
Panic in get_host_func if the async-ness mismatches.

I think I'd lean towards the first option if we could, I don't think there's actually any use for async being at the Store level instead of Config, that's just originally how it played out. While it's technically possible to mix stores in the same engine with different async configurations, I'm not sure if there's much practical use to that.

fitzgen · 2021-03-09T21:42:49Z

I think I'd lean towards the first option if we could,

+1

peterhuene · 2021-03-09T22:11:04Z

That works for me too. I'll see about implementing that. I'm also adding Config::define_host_func_async and updating the async function tests to work with both Func and these shared host functions.

This commit moves the concept of "async-ness" to `Config` rather than `Store`. Note: this is a breaking API change for anyone that's already adopted the new async support in Wasmtime. Now `Config::new_async` is used to create an "async" config and any `Store` associated with that config is inherently "async". This is needed for async shared host functions to have some sanity check during their execution (async host functions, like "async" `Func`, need to be called with the "async" variants).

This commit updates the async function tests to also smoke the shared host functions, plus `Func::wrap0_async`. This also changes the "wrap async" method names on `Config` to `wrap$N_host_func_async` to slightly better match what is on `Func`.

peterhuene · 2021-03-10T00:42:32Z

@alexcrichton I pushed up a few more commits, including moving the "async-ness" to Config.

alexcrichton

I'm curious on your thoughts on new_async vs Config::async_ as a configurator, but otherwise can you update the WASI example as well to use the add_to_config method instead of the add_to_linker?

I'd be ok either outright removing add_to_linker, but in general it seems like we should nudge folks to add_to_config if possible.

crates/wasmtime/src/config.rs

peterhuene · 2021-03-10T18:24:33Z

I'll update the WASI example to define the host functions in Config.

I think we will want to leave add_to_linker in place, as it does provide a use case that add_to_config does not: using a WASI context for a particular subset of instances rather than sharing the context for all instances in the store.

This commit moves the instantiated instance allocator from `Config` into `Engine`. This makes certain settings in `Config` no longer order-dependent, which is how `Config` should ideally be. This also removes the confusing concept of the "default" instance allocator, instead opting to construct the on-demand instance allocator when needed. This does alter the semantics of the instance allocator as now each `Engine` gets its own instance allocator rather than sharing a single one between all engines created from a configuration.

alexcrichton · 2021-03-10T20:05:52Z

I like the most recent commit and think it works well. I'd probably change Engine::new to return a Result though so we can avoid panicking. The Engine::default() method is known to not be fallible though so seems reasonable to continue to panic there.

I agree with you, though, that Config is definitely becoming perhaps a bit too stateful. I'm not sure how deferring creation of the allocator, though, changes that? Or is it basically saying that the host functions are the only bit of state remaining?

peterhuene · 2021-03-10T20:13:01Z

I'm +1 on Engine::new returning Result and Engine::default panicking. It's a pretty big breaking change though.

I just meant that deferring the instance allocator instantiation helps Config be less stateful as now we don't have to track what settings the pooling instance allocator depends on and therefore ensure they don't change after its creation (deferring makes that a nonissue).

Host functions would be the last thing contributing to the state of Config such that we need to track a setting dependency like this (in this case it's just whether "async" is enabled, but still).

alexcrichton · 2021-03-10T20:27:21Z

Eh I'm not too worried about the breaking change here, in theory it's O(1) per embedding so while it's a big change for us since we have so many tests for consumers it's probably not that bad. (I also have other desired breaking changes like #2719 which are large-ish)

I suppose another thing we could do is go the route of Engine::new(&config, &funcs) or something like that where that way we'd just validate whether config and funcs agree on async-ness. Alternatively we could never return an error on mixed functions being added to a Config, but the error is returned on Engine::new?

TBH I'm not sure I know of a great way to manage this, but I don't think it matters too much. We don't have to get it 100% right just yet so as long as something works pretty well I think it's fine to land.

This is a breaking API change for anyone using `Engine::new`. As creating the pooling instance allocator may fail (likely cause is not enough memory for the provided limits), instead of panicking when creating an `Engine`, `Engine::new` now returns a `Result`.

This commit removes `Config::new_async` in favor of treating "async support" as any other setting on `Config`. The setting is `Config::async_support`.

peterhuene · 2021-03-10T23:58:47Z

So I've removed Config::new_async and replaced it with a Config::async_support (async_ I'm not a fan of just to get around the use of a keyword) setting.

I've also (a little hackily imo) deferred the check of async shared host functions requiring async_support(true) to engine creation so that users now can enable async support after defining host functions in the config and vice versa without us having to introduce an order dependency.

This commit removes the order dependency where async support must be enabled on the `Config` prior to defining async host functions. The check is now delayed to when an `Engine` is created from the config.

This commit updates the WASI example to use `Wasi::add_to_config`. As only a single store and instance are used in the example, it has no semantic difference from the previous example, but the intention is to steer users towards defining WASI on the config and only using `Wasi::add_to_linker` when more explicit scoping of the WASI context is required.

github-actions · 2021-03-11T02:03:35Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

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

Learn more.

alexcrichton · 2021-03-11T16:14:07Z

👍

github-actions bot added wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself labels Jan 30, 2021

alexcrichton reviewed Feb 1, 2021

View reviewed changes

peterhuene changed the title ~~Implement defining functions at the Engine level.~~ Implement shared host functions. Feb 2, 2021

peterhuene force-pushed the engine-funcs branch 2 times, most recently from 7c1b8ae to c10bf06 Compare February 2, 2021 02:59

peterhuene requested review from alexcrichton and fitzgen February 2, 2021 03:02

peterhuene force-pushed the engine-funcs branch from c10bf06 to 3144f59 Compare February 2, 2021 03:15

alexcrichton reviewed Feb 2, 2021

View reviewed changes

crates/wasmtime/src/linker.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/func.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/engine.rs Outdated Show resolved Hide resolved

fitzgen reviewed Feb 2, 2021

View reviewed changes

crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/store.rs Outdated Show resolved Hide resolved

peterhuene force-pushed the engine-funcs branch from 26ea121 to 454276b Compare February 2, 2021 23:05

peterhuene requested a review from alexcrichton February 2, 2021 23:07

alexcrichton approved these changes Feb 2, 2021

View reviewed changes

peterhuene force-pushed the engine-funcs branch from 4792401 to 3c43ed4 Compare February 3, 2021 19:40

peterhuene force-pushed the engine-funcs branch from 000bf5e to 4a34973 Compare February 19, 2021 01:00

pepyakin mentioned this pull request Mar 3, 2021

sc-executor-wasmtime: leverage shared host functions paritytech/substrate#8252

Open

peterhuene force-pushed the engine-funcs branch from 4a34973 to fe40cfa Compare March 9, 2021 03:01

github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Mar 9, 2021

peterhuene force-pushed the engine-funcs branch from fe40cfa to 34cbd8b Compare March 9, 2021 03:47

peterhuene force-pushed the engine-funcs branch from 34cbd8b to 3ada48c Compare March 9, 2021 20:53

peterhuene force-pushed the engine-funcs branch from 3ada48c to a284c00 Compare March 9, 2021 20:58

alexcrichton approved these changes Mar 9, 2021

View reviewed changes

peterhuene added 3 commits March 9, 2021 14:18

Add Config::define_host_func_async.

8d9d4d8

alexcrichton approved these changes Mar 10, 2021

View reviewed changes

crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved

peterhuene force-pushed the engine-funcs branch from 6223144 to 0e70704 Compare March 10, 2021 19:57

peterhuene added 2 commits March 10, 2021 12:58

Remove Config::new_async.

3aa0bfd

This commit removes `Config::new_async` in favor of treating "async support" as any other setting on `Config`. The setting is `Config::async_support`.

peterhuene added 2 commits March 10, 2021 16:08

Remove order dependency when defining async host functions in Config.

7bd3533

This commit removes the order dependency where async support must be enabled on the `Config` prior to defining async host functions. The check is now delayed to when an `Engine` is created from the config.

peterhuene force-pushed the engine-funcs branch from 7e96169 to f1f5443 Compare March 11, 2021 00:09

github-actions bot added the fuzzing Issues related to our fuzzing infrastructure label Mar 11, 2021

alexcrichton merged commit 54c07d8 into bytecodealliance:main Mar 11, 2021

peterhuene deleted the engine-funcs branch March 11, 2021 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement shared host functions. #2625

Implement shared host functions. #2625

peterhuene commented Jan 30, 2021 •

edited

Loading

github-actions bot commented Jan 30, 2021

fitzgen left a comment

peterhuene commented Mar 8, 2021

peterhuene commented Mar 9, 2021 •

edited

Loading

github-actions bot commented Mar 9, 2021

alexcrichton commented Mar 9, 2021

peterhuene commented Mar 9, 2021

peterhuene commented Mar 9, 2021

alexcrichton left a comment

fitzgen commented Mar 9, 2021

peterhuene commented Mar 9, 2021

peterhuene commented Mar 10, 2021

alexcrichton left a comment

peterhuene commented Mar 10, 2021 •

edited

Loading

alexcrichton commented Mar 10, 2021

peterhuene commented Mar 10, 2021

alexcrichton commented Mar 10, 2021

peterhuene commented Mar 10, 2021

github-actions bot commented Mar 11, 2021

alexcrichton commented Mar 11, 2021

Implement shared host functions. #2625

Implement shared host functions. #2625

Conversation

peterhuene commented Jan 30, 2021 • edited Loading

github-actions bot commented Jan 30, 2021

Subscribe to Label Action

fitzgen left a comment

Choose a reason for hiding this comment

peterhuene commented Mar 8, 2021

peterhuene commented Mar 9, 2021 • edited Loading

github-actions bot commented Mar 9, 2021

Subscribe to Label Action

alexcrichton commented Mar 9, 2021

peterhuene commented Mar 9, 2021

peterhuene commented Mar 9, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

fitzgen commented Mar 9, 2021

peterhuene commented Mar 9, 2021

peterhuene commented Mar 10, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

peterhuene commented Mar 10, 2021 • edited Loading

alexcrichton commented Mar 10, 2021

peterhuene commented Mar 10, 2021

alexcrichton commented Mar 10, 2021

peterhuene commented Mar 10, 2021

github-actions bot commented Mar 11, 2021

Subscribe to Label Action

alexcrichton commented Mar 11, 2021

peterhuene commented Jan 30, 2021 •

edited

Loading

peterhuene commented Mar 9, 2021 •

edited

Loading

peterhuene commented Mar 10, 2021 •

edited

Loading