Reactor support. #1565

sunfishcode · 2020-04-21T14:32:25Z

This implements the new WASI ABI described here:

https://github.com/WebAssembly/WASI/blob/master/design/application-abi.md

It adds APIs to Instance and Linker with support for running
WASI programs, and also simplifies the process of instantiating
WASI API modules.

This currently only includes Rust API support.

github-actions · 2020-04-21T14:58:59Z

Subscribe to Label Action

cc @kubkon, @peterhuene

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

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.

RELEASES.md

alexcrichton · 2020-04-21T17:29:57Z

crates/api/src/instance.rs

+    ///    the initialized `Instance` is returned.
+    ///
+    /// [WASI ABI initialization]: https://github.com/WebAssembly/WASI/blob/master/design/application-abi.md#current-unstable-abi
+    pub fn new_wasi_abi(module: &Module, imports: &[Extern]) -> Result<Option<Instance>, Error> {


I think this API makes sense given WASI today, but this feels sort of uncomfortable. This means that we're splitting the world between "instantiate this module" and "instantiate this wasi module". These constructor semantics need to be bubbled to all callers and such.

Would it be possible to avoid splitting into two worlds? One option might be to have this instead of a constructor:

impl Instance { fn initialize_wasi(self) -> Result<Option<Instance>, Error> { // ... } }

That way you'd always create the Instance the same way and it's up to you what to do with it after that.

I'm imagining new_wasi_abi will be a common case. The name is perhaps confusing because this doesn't mean "run with all the WASI APIs available", but just "run the WASI startup functions if present". I actually wonder if we should rename this to new and rename the current new to new_raw or something, to emphasize that it's not automatically running the WASI startup functions (though it does still run the wasm start function).

I personally still feel that this is not the API we want. Adding a new constructor for instances which "does the wasi thing" is infectious and will continue to force us to duplicate API surface area into the future. I feel remembering to call new_wasi_abi is basically equivalent to remembering to call initialize_wasi, and in terms of composability I feel that an explicit function to initialize is more convenient.

A lucet change to disconnect initialization from instantiation also points in this direction.

I experimented with a few different API approaches to this, and came up with the patch that's now pushed here. With this, the Rust API's Instance::new and Linker::instantiate return a NewInstance, which is a wrapper around Instance where the programmer has to initialize it before getting the Instance out. We may need to do something different for the C API though.

src/commands/run.rs

crates/test-programs/tests/wasm_tests/runtime.rs

crates/wasi/src/lib.rs

examples/wasi/main.rs

github-actions · 2020-05-18T23:02:36Z

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 · 2020-05-20T14:24:08Z

I do like this approach better myself, but I feel like it's still a bit too heavyweight because it makes NewInstance almost exclusively WASI-focused. I would expect Instance to be neutral with respect to WASI-or-not and users have to opt-in to WASI-specific hooks.

I realize it can be a small footgun if these methods are added directly to Instance, but I feel that the footgun is worth it in terms of ergonomics. For example if you're reading the documentation of the wasmtime crate all the examples have init_reactor(&[]) which is somewhat bizarre if you've never heard of WASI before.

sunfishcode · 2020-05-20T20:38:32Z

They aren't really WASI-specific. I expect that between the linking spec and interface types, the definition of commands and reactors will eventually want to move out of WASI and into one of those other specs. Would it help if we moved the definition into tool-conventions for now? We could then avoid calling these "WASI" functions. Would that help avoid confusion?

I'm also not tied to the name init_reactor. Would .initialize(&[]) or something else look less surprising?

github-actions · 2020-05-20T23:19:34Z

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 · 2020-05-21T14:18:00Z

Oh sorry, so I mean to say that the current as-is state of NewInstance feels very WASI-specific. The options you have are:

start - this is actually somewhat unrelated to the wasm start function and classifies every "normal" wasm module as a "reactor"
run_command, init_reactor - WASI-specific

There's not really an easy option for "just give me the Instance I don't care about WASI". Even with init_reactor you have to pass an array of arguments which can be confusing.

This is why I still think it's best to return an work with an Instance. The type-level distinction of a NewInstance I don't think is buying us much here. If we really want we could always probe what an Instance looks like and dynamically guard invocations of functions. We could say, for example, that you can't call WASI functions before you call the wasi initialization function which does the command/reactor thing.

This implements the new WASI ABI described here: https://github.com/WebAssembly/WASI/blob/master/design/application-abi.md It adds APIs to `Instance` and `Linker` with support for running WASI programs, and also simplifies the process of instantiating WASI API modules. This currently only includes Rust API support.

This also separates instantiation from initialization in a manner similar to bytecodealliance/lucet#506.

…ted to WASI.

sunfishcode · 2020-05-22T21:59:32Z

Ok, I've now gone back to Instance::new not doing any command/reactor stuff. I kept NewInstance, with a start function that now just runs the wasm start function. That also simplifies the C API story, so this patch now has C API updates corresponding to the Rust API updates.

The new approach now adds a new function Linker::module, which handles instantiating the module, automatically handling command/reactor semantics, which ends up being a lot nicer because we don't need APIs that conditionally consume an Instance. Linker::module avoids the user having to deal with Instances altogether.

alexcrichton

Nice! I think the usage of Linker here is pretty slick.

Would it be possible to add some API-level tests for the new Linker methods too?

src/commands/run.rs

crates/wasmtime/src/instance.rs

crates/wasmtime/src/linker.rs

alexcrichton · 2020-05-22T22:51:15Z

crates/wasmtime/src/linker.rs

+    ///
+    /// An export with an empty string is considered to be a "default export".
+    /// "_start" is also recognized for compatibility.
+    pub fn get_default(&self, module: &str) -> Result<Func> {


This function feels a bit odd to me because the notion of a "default" export doesn't seems super well defined. Is there WASI documentation this could link to?

In isolation it seems like this API should be on an Instance as well, but I understand why it's here instead of instances at this time. Perhaps they could share an implementation of sorts?

Also since this is WASI-specific would it make sense to do something like get_wasi_default()?

alexcrichton · 2020-05-22T22:52:20Z

crates/wasmtime/src/linker.rs

@@ -270,6 +271,117 @@ impl Linker {
        Ok(self)
    }

+    /// Define automatic instantiations of a [`Module`] in this linker.


I think this should highlight very clearly that that the major purpose of this method is to transparently handle WASI conventions as well, e.g. "WASI" should be somewhere in this heading.

I'm actually trying to de-emphasize "WASI" here. Commands and Reactors are currently defined in a WASI-repository page, but they're not related to WASI APIs, and I expect they'll eventually move out into the linking and/or interface-types specs.

alexcrichton · 2020-05-22T22:55:02Z

src/commands/run.rs

-    }
-
-    fn handle_module(&self, store: &Store, module_registry: &ModuleRegistry) -> Result<()> {
+    fn load_main_module(&self, linker: &mut Linker) -> Result<()> {
        if let Some(timeout) = self.wasm_timeout {


This is a "preexisting" bug but we should probably at some point spawn this thread before we start instantiating anything to account for start functions and _initialize and such.

(although ideally still discounting compile times of Module)

Anyway, not something to handle in this PR, just a thought.

Good point. Perhaps, if caching is enabled and a timeout is requested, we can prime the cache with module compilations, and then start the timer before we start the Linker. If caching is disabled, then we just count compilation time toward the timeout, perhaps.

alexcrichton · 2020-05-22T22:56:16Z

src/commands/run.rs

+        // Use "" as a default module name.
+        let module = Module::from_file(linker.store(), &self.module)?;
+        linker
+            .module("", &module)


While probably not a huge issue it's a bit odd that we have to assign a name to the instance here, since ideally we'd pull out an instance and "do the wasi thing" on it.

I used a name here because I think src/commands/run.rs shouldn't be pulling out the instance and doing the initialization manually. The Linker handles that for it, which is good because then the functionality is available to API users as well as command-line users.

alexcrichton · 2020-05-26T15:39:36Z

Ok this all looks great to me, thanks again for this @sunfishcode! I feel like anything further we can continue to iterate on in-tree if necessary, so I'm gonna merge.

sunfishcode added the wasmtime Issues about wasmtime that don't fall into another label label Apr 21, 2020

github-actions bot added wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:docs Issues related to Wasmtime's documentation labels Apr 21, 2020

alexcrichton reviewed Apr 21, 2020

View reviewed changes

sunfishcode force-pushed the reactor branch from 01db83a to 1f37f74 Compare May 15, 2020 01:09

abrown reviewed May 15, 2020

View reviewed changes

crates/test-programs/tests/wasm_tests/runtime.rs Outdated Show resolved Hide resolved

abrown reviewed May 15, 2020

View reviewed changes

crates/wasi/src/lib.rs Outdated Show resolved Hide resolved

abrown reviewed May 15, 2020

View reviewed changes

examples/wasi/main.rs Outdated Show resolved Hide resolved

sunfishcode force-pushed the reactor branch from aa8c1f5 to a6ab235 Compare May 18, 2020 22:47

github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label May 18, 2020

github-actions bot added the fuzzing Issues related to our fuzzing infrastructure label May 20, 2020

sunfishcode force-pushed the reactor branch from aa369af to 87b2592 Compare May 21, 2020 17:53

sunfishcode added 9 commits May 22, 2020 09:31

Add comments and fix a typo in a comment.

aa540eb

Fix a rustdoc warning.

feaf369

Tidy an unneeded mut.

3d5aa57

Factor out instance initialization with NewInstance.

99fa99d

This also separates instantiation from initialization in a manner similar to bytecodealliance/lucet#506.

Update fuzzing oracles for the API changes.

963d1bd

Remove wasi_linker and clarify that Commands/Reactors aren't connec…

41ba139

…ted to WASI.

Move Command/Reactor semantics into the Linker.

39c46d4

C API support.

b9b912c

sunfishcode force-pushed the reactor branch from 87b2592 to b9b912c Compare May 22, 2020 21:50

Fix fuzzer build.

07ebb32

alexcrichton reviewed May 22, 2020

View reviewed changes

sunfishcode added 8 commits May 22, 2020 16:01

Update usage syntax from "::" to "=".

1d71646

Remove NewInstance and start().

fdd63a5

Elaborate on Commands and Reactors and add a spec link.

6b235b2

Add more comments.

ad8de0d

Fix wat syntax.

2ad2d41

Fix wat.

d73a96c

Use the Debug formatter to format an anyhow::Error.

53ebb7f

Fix wat.

4a6ce8e

alexcrichton merged commit 3715e19 into bytecodealliance:master May 26, 2020

sunfishcode deleted the reactor branch June 2, 2020 00:26

alexcrichton mentioned this pull request Jul 14, 2020

Jitdump support sometimes doesn't work #2018

Closed

kazum mentioned this pull request Nov 9, 2020

[CI] Install libc6-dev-i386 to compile wasm32 apache/tvm#6886

Merged

SingleAccretion mentioned this pull request Aug 2, 2023

NativeAOT-LLVM: Init callback declaration dotnet/runtimelab#2364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reactor support. #1565

Reactor support. #1565

sunfishcode commented Apr 21, 2020

github-actions bot commented Apr 21, 2020

alexcrichton Apr 21, 2020

sunfishcode May 15, 2020

alexcrichton May 15, 2020

sunfishcode May 18, 2020

github-actions bot commented May 18, 2020

alexcrichton commented May 20, 2020

sunfishcode commented May 20, 2020

github-actions bot commented May 20, 2020

alexcrichton commented May 21, 2020

sunfishcode commented May 22, 2020

alexcrichton left a comment

alexcrichton May 22, 2020

alexcrichton May 22, 2020

alexcrichton May 22, 2020

sunfishcode May 22, 2020

alexcrichton May 22, 2020

alexcrichton May 22, 2020

sunfishcode May 23, 2020

alexcrichton May 22, 2020

sunfishcode May 23, 2020

alexcrichton commented May 26, 2020

Reactor support. #1565

Reactor support. #1565

Conversation

sunfishcode commented Apr 21, 2020

github-actions bot commented Apr 21, 2020

Subscribe to Label Action

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 18, 2020

Subscribe to Label Action

alexcrichton commented May 20, 2020

sunfishcode commented May 20, 2020

github-actions bot commented May 20, 2020

Subscribe to Label Action

alexcrichton commented May 21, 2020

sunfishcode commented May 22, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented May 26, 2020