wasmtime / PR #1384 Add a `wasmtime::Linker` type · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1384 Add a `wasmtime::Linker` type

Wasmtime GitHub notifications bot (Mar 23 2020 at 16:22):

alexcrichton opened PR #1384 from linking to master:

This commit adds a new type to the wasmtime crate, a Linker. This
linker is intended to vastly simplify calling Instance::new by easily
performing name resolution and incrementally defining state over time.
The goal here is to start down a path of making linking wasm modules in
wasmtime a first-class and ergonomic operation. This is highly likely
to evolve over time and get tweaked through releases as we iterate
towards a design well-suited for wasmtime, but this is intended to at
least be the initial foundation for such functionality.

This commit additionally also adds a C API for the linker and switches
the existing linking examples to using this linker in both Rust and C.

One piece of future work I'd like to tackle next is to integrate WASI
into the wasmtime crate in a more first-class manner. This [Linker]
type provides a great location to hook into the instantiation process to
easily instantiate modules with WASI imports. That's a relatively large
refactoring for now though and I figured it'd be best left for a
different time.

Closes #727

Wasmtime GitHub notifications bot (Mar 23 2020 at 16:47):

alexcrichton updated PR #1384 from linking to master:

This commit adds a new type to the wasmtime crate, a Linker. This
linker is intended to vastly simplify calling Instance::new by easily
performing name resolution and incrementally defining state over time.
The goal here is to start down a path of making linking wasm modules in
wasmtime a first-class and ergonomic operation. This is highly likely
to evolve over time and get tweaked through releases as we iterate
towards a design well-suited for wasmtime, but this is intended to at
least be the initial foundation for such functionality.

This commit additionally also adds a C API for the linker and switches
the existing linking examples to using this linker in both Rust and C.

One piece of future work I'd like to tackle next is to integrate WASI
into the wasmtime crate in a more first-class manner. This [Linker]
type provides a great location to hook into the instantiation process to
easily instantiate modules with WASI imports. That's a relatively large
refactoring for now though and I figured it'd be best left for a
different time.

Closes #727

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene created PR Review Comment:

It's a little odd to me that Linker::instantiate needs to be &mut self. Perhaps an Linker::insert_key and Linker::get_key method, where the former does what Linker::import_key is doing now and the latter returns Option<ImportKey> after checking if the given strings are present in the data structures without interning? That would remove the requirement of this being &mut self.

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene created PR Review Comment:

Should this (and the corresponding Rust impl) be const wasm_instance_t*?

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene created PR Review Comment:

Should this (and the corresponding Rust impl) be const wasi_instance_t*?

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene created PR Review Comment:

WASM_API_EXTERN own wasmtime_linker_t* wasmtime_linker_new(wasm_store_t* store);

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene created PR Review Comment:

Assuming Linker::instantiate can become &self, this (and the corresponding Rust impl) should probably be const wasmtime_linker_t *.

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene created PR Review Comment:

Extreme nit: record_instantiate implies a record is kept some somewhere to me. Perhaps handle_instantiation since it takes Result<Instance>?

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene created PR Review Comment:

Should this (and the corresponding Rust impl) be const wasm_module_t*?

Wasmtime GitHub notifications bot (Mar 23 2020 at 21:45):

peterhuene edited PR Review Comment.

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

In general builders in Rust have &mut self even on the build methods, primarily for future-flexibility. That being said given the way I envision this being used I think we'll want a guarantee that &mut self isn't needed and that mutations don't happen. In that sense I agree, let's shoot for &self here.

Wasmtime GitHub notifications bot (Mar 23 2020 at 23:04):

alexcrichton updated PR #1384 from linking to master:

This commit adds a new type to the wasmtime crate, a Linker. This
linker is intended to vastly simplify calling Instance::new by easily
performing name resolution and incrementally defining state over time.
The goal here is to start down a path of making linking wasm modules in
wasmtime a first-class and ergonomic operation. This is highly likely
to evolve over time and get tweaked through releases as we iterate
towards a design well-suited for wasmtime, but this is intended to at
least be the initial foundation for such functionality.

This commit additionally also adds a C API for the linker and switches
the existing linking examples to using this linker in both Rust and C.

One piece of future work I'd like to tackle next is to integrate WASI
into the wasmtime crate in a more first-class manner. This [Linker]
type provides a great location to hook into the instantiation process to
easily instantiate modules with WASI imports. That's a relatively large
refactoring for now though and I figured it'd be best left for a
different time.

Closes #727

Wasmtime GitHub notifications bot (Mar 23 2020 at 23:50):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 01:29):

alexcrichton updated PR #1384 from linking to master:

This commit adds a new type to the wasmtime crate, a Linker. This
linker is intended to vastly simplify calling Instance::new by easily
performing name resolution and incrementally defining state over time.
The goal here is to start down a path of making linking wasm modules in
wasmtime a first-class and ergonomic operation. This is highly likely
to evolve over time and get tweaked through releases as we iterate
towards a design well-suited for wasmtime, but this is intended to at
least be the initial foundation for such functionality.

This commit additionally also adds a C API for the linker and switches
the existing linking examples to using this linker in both Rust and C.

One piece of future work I'd like to tackle next is to integrate WASI
into the wasmtime crate in a more first-class manner. This [Linker]
type provides a great location to hook into the instantiation process to
easily instantiate modules with WASI imports. That's a relatively large
refactoring for now though and I figured it'd be best left for a
different time.

Closes #727

Wasmtime GitHub notifications bot (Mar 24 2020 at 02:02):

alexcrichton merged PR #1384.

Wasmtime GitHub notifications bot (Mar 24 2020 at 19:19):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 19:19):

sunfishcode created PR Review Comment:

This should have a ? to handle errors.

Wasmtime GitHub notifications bot (Mar 24 2020 at 19:27):

alexcrichton created PR Review Comment:

Oh good eye, I've added a commit to https://github.com/bytecodealliance/wasmtime/pull/1391 which denies warnings in doctests to catch this

Wasmtime GitHub notifications bot (Mar 24 2020 at 19:27):

alexcrichton submitted PR Review.

Last updated: Nov 22 2024 at 16:03 UTC