Stream: git-wasmtime

Topic: wasmtime / Issue #2625 Implement shared host functions.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2021 at 21:55):

peterhuene commented on Issue #2625:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 03:02):

peterhuene commented on Issue #2625:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 03:05):

peterhuene edited a comment on Issue #2625:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 03:45):

github-actions[bot] commented on Issue #2625:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 03:47):

peterhuene edited a comment on Issue #2625:

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).

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 15:12):

alexcrichton commented on Issue #2625:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 19:35):

peterhuene commented on Issue #2625:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 20:47):

peterhuene commented on Issue #2625:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 21:42):

fitzgen commented on Issue #2625:

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

+1

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2021 at 22:11):

peterhuene commented on Issue #2625:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2021 at 00:42):

peterhuene commented on Issue #2625:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2021 at 18:24):

peterhuene commented on Issue #2625:

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 instance rather than sharing the context for all instances in the store.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2021 at 18:28):

peterhuene edited a comment on Issue #2625:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2021 at 20:05):

alexcrichton commented on Issue #2625:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2021 at 20:13):

peterhuene commented on Issue #2625:

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).

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2021 at 20:27):

alexcrichton commented on Issue #2625:

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 https://github.com/bytecodealliance/wasmtime/pull/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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2021 at 23:58):

peterhuene commented on Issue #2625:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 11 2021 at 02:03):

github-actions[bot] commented on Issue #2625:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Mar 11 2021 at 16:14):

alexcrichton commented on Issue #2625:

:+1:

Last updated: Nov 22 2024 at 17:03 UTC