wasmtime / PR #1565 Reactor support. · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1565 Reactor support.

Wasmtime GitHub notifications bot (Apr 21 2020 at 14:32):

sunfishcode opened PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (Apr 21 2020 at 17:34):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 21 2020 at 17:34):

alexcrichton created PR Review Comment:

Duplicated sections?

Wasmtime GitHub notifications bot (Apr 21 2020 at 17:34):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 21 2020 at 17:34):

alexcrichton created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Apr 21 2020 at 17:34):

alexcrichton created PR Review Comment:

Could you add a test for this new behavior too?

Also, perhaps for some ergonomic, could this be something like:

foo.wasm - parses as module name foo and path foo.wasm

foo/bar.wasm - parses as module name bar and path name foo/bar.wasm

foo=bar.wasm - parses as module name foo and path name bar.wasm

(the = vs :: is certainly a bikeshed, but I personally would lean towards =, not that it matters too too much)

Wasmtime GitHub notifications bot (May 15 2020 at 01:09):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 15 2020 at 01:14):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 01:14):

sunfishcode created PR Review Comment:

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

Wasmtime GitHub notifications bot (May 15 2020 at 01:19):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 01:19):

sunfishcode created PR Review Comment:

I've renamed "::" to "=", and added a few tests that use it. I haven't done foo.wasm or foo/bar.wasm yet; I need to think about those a little more first.

Wasmtime GitHub notifications bot (May 15 2020 at 02:40):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 02:40):

abrown created PR Review Comment:

At first glance this looks like it should be if instance.is_none() ... (it looks like error-checking) but now I see that an Instance shouldn't be returned for commands--an explanatory comment might help?

Wasmtime GitHub notifications bot (May 15 2020 at 02:41):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 02:41):

abrown created PR Review Comment:

Why do we preopen dirs twice?

Wasmtime GitHub notifications bot (May 15 2020 at 02:42):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 02:42):

abrown created PR Review Comment:

Instanciate -> Instantiate

Wasmtime GitHub notifications bot (May 15 2020 at 03:58):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 15 2020 at 03:58):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 03:58):

sunfishcode created PR Review Comment:

Once for snapshot1 and once for snapshot0. I've now added a comment about this.

Wasmtime GitHub notifications bot (May 15 2020 at 03:59):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 03:59):

sunfishcode created PR Review Comment:

Comment added.

Wasmtime GitHub notifications bot (May 15 2020 at 04:01):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 15 2020 at 21:09):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 15 2020 at 21:09):

alexcrichton created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 18 2020 at 22:45):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 18 2020 at 22:47):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 18 2020 at 23:00):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 18 2020 at 23:00):

sunfishcode created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 20 2020 at 22:33):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 21 2020 at 17:53):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 22 2020 at 21:50):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 22 2020 at 22:29):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

I think this :: should be = now?

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

Is NewInstance still necessary here? I'd imagine that 95% of invocations of Instance::new will immediately want to .start()? aftewards (and seems like 100% in this PR?). Do we have a use case for eventually getting an Instance but not running the start function?

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

The terms "reactor" and "command" I think are pretty new to the documentation here. Would it be possible to link to somewhere else that has more details? This could also clarify that non-WASI-specific modules are handled here too, and they're treated similarly as to reactors.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

I think this section may want to also get expanded with information about how the Linker needs to already contain all items necessary to instantiate the module when this function is called, even if this is a command.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

I think in general though these two paragraphs about what happens for reactor/commands could be expanded to describe in a bit more detail what's happening under the hood.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

These and below are definitely a bummer, but I guess there's not much we can do about it?

How come though we validate that only functions and a few whitelisted things are exported?

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

Would it be possible to include an inline example of a command and how it's a new instance each time?

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

This I think may want to use the debug formatter instead of Display to capture the full context of the anyhow::Error

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

Could a small inline comment be added above this whole statement indicating why all these .unwrap()s aren't expected to fail?

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

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

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

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?

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

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

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 22 2020 at 22:58):

alexcrichton created PR Review Comment:

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)

Wasmtime GitHub notifications bot (May 22 2020 at 23:21):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 22 2020 at 23:21):

sunfishcode created PR Review Comment:

I guess you're right. I was initially thinking of https://github.com/bytecodealliance/lucet/pull/506, but on reflection, I think this will become less important over time. If we ever do need this, we can always add a separate Instance constructor to construct a partially-initialized Instance.

Wasmtime GitHub notifications bot (May 22 2020 at 23:30):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 22 2020 at 23:31):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 22 2020 at 23:31):

sunfishcode created PR Review Comment:

I've now expanded this comment accordingly.

Wasmtime GitHub notifications bot (May 22 2020 at 23:58):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 22 2020 at 23:58):

sunfishcode created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 23 2020 at 00:07):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 23 2020 at 00:07):

sunfishcode created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 23 2020 at 00:08):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 23 2020 at 00:09):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 23 2020 at 00:09):

sunfishcode created PR Review Comment:

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.

Wasmtime GitHub notifications bot (May 23 2020 at 00:22):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (May 23 2020 at 00:22):

sunfishcode created PR Review Comment:

Rust is one of the toolchains using --export-dynamic and I'd ideally like to remove that, but I need to figure out exactly how Rust use cases are depending on that before we can remove it. I've heard of other toolchains using it as well, as it sort of gives people what they want -- "just export my stuff". Except that --export-dynamic was implemented in wasm-ld to handle the Emscripten-specific dynamic linking scheme used in Emscripten, so it also does some additional things that Emscripten needs like exporting __heap_base and __data_end, even when we aren't using Emscripten. So we have a multi-project backwards compatibility knot to untie before we can fix this.

In general, it doesn't make sense for Commands to export globals, tables, or memories, because those are all about state, and Commands don't have any state, except when one of their exported functions is being called.

Wasmtime GitHub notifications bot (May 23 2020 at 00:25):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 23 2020 at 02:06):

sunfishcode updated PR #1565 from reactor to master:

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.

Wasmtime GitHub notifications bot (May 26 2020 at 15:38):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 26 2020 at 15:38):

alexcrichton created PR Review Comment:

Ah yeah our usage of LLD is very dated at this point in rustc. At least for rustc it probably won't be too too hard to fix this I think.

Wasmtime GitHub notifications bot (May 26 2020 at 15:39):

alexcrichton merged PR #1565.

Last updated: Nov 22 2024 at 16:03 UTC