rewrite gen-host-wasmtime-rust to use Wasmtime's component runtime #355
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pchickey
force-pushed
the
host-wasmtime-rust
branch
from
922df99 to
b405ec6
Compare
added 28 commits
October 10, 2022 17:29
we need a git wasmtime version to have an up-to-date wasmparser inside. the 1.0.1 release (latest at this moment) wasmparser chokes on basic stuff
so that wasi-libc can initialize args and env as empty in rust guest
so that the exported interface isnt overwritten when one is not found inside the module.
otherwise llvm requires 16 pages of imported memory, which will conflict when linking against modules created by teavm which have a smaller minimum memory size
still some WIP on the flags macro over in wasmtime required
Wasmtime decided that 16 is the limit of the ComponentNamedList tuple length. This limit is arbitrary, so I shrunk the test rather than raise wasmtime's limit.
i am skeptical about the python embedding, these tests shouldnt have passed before
pchickey
force-pushed
the
host-wasmtime-rust
branch
from
a65b80c to
720aa06
Compare
added 6 commits
October 10, 2022 18:11
the one for enum discriminant is worse than before, i guess
There was a problem hiding this comment.
This looks great to me, thanks again for this! I'm quite pleased at how simple the Wasmtime generator ended up being (or at least removing of now-vestigal complexity).
There's not a ton to say about the rewrite. The tests show that it works.
I'm glad the tests worked well for this!
I departed from the previous conventions in this crate to follow the behaviors available from the Wasmtime component runtime:
Can you file an issue on merging to improve this? I don't want to lose track of this but I do think that what you've implemented here is suitable for the time being. I think it's best to learn more about embeddings and what traits are expected given to guide the design here rather than the prior implementation which "maximized borrows where possible".
I do think we'll need something in this area to make this truly viable, especially to avoid copies with parameters like (list u8) given to a print-like function. This is all fine to defer to later though.
|
|
||
| // This is a pretty naive way to account for borrows. This datastructure | ||
| // could be made a lot more efficient with some effort. | ||
| pub struct BorrowChecker<'a> { |
There was a problem hiding this comment.
I'm so happy we get to remove this
added 3 commits
October 12, 2022 09:52
the flavorful/host.rs shows you can enable it explicitly, but its
actually enabled for all tests because the cargo feature is turned on.
[phickey@pch-tower:src/wit-bindgen]% RUST_LOG=trace RUST_LOG_SPAN_EVENTS=full cargo test -p wit-bindgen-gen-host-wasmtime-rust -- records_rust --nocapture
Finished test [unoptimized + debuginfo] target(s) in 0.10s
Running tests/codegen.rs (target/debug/deps/codegen-a87f1b59880af895)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 68 filtered out; finished in 0.00s
Running tests/runtime.rs (target/debug/deps/runtime-e18eabec1fbb6c9b)
running 1 test
2022-10-12T17:50:48.399919Z TRACE wit-bindgen import{module="exports" function="test-imports"}: runtime::records_rust::exports: new
2022-10-12T17:50:48.400036Z TRACE wit-bindgen import{module="exports" function="test-imports"}: runtime::records_rust::exports: enter
2022-10-12T17:50:48.400380Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="multiple-results"}: runtime::records_rust::imports: new
2022-10-12T17:50:48.400449Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="multiple-results"}: runtime::records_rust::imports: enter
2022-10-12T17:50:48.400522Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="multiple-results"}: runtime::records_rust::imports: exit
2022-10-12T17:50:48.400575Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="multiple-results"}: runtime::records_rust::imports: close time.busy=73.0µs time.idle=140µs
2022-10-12T17:50:48.400772Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="swap-tuple"}: runtime::records_rust::imports: new
2022-10-12T17:50:48.400834Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="swap-tuple"}: runtime::records_rust::imports: enter
2022-10-12T17:50:48.400890Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="swap-tuple"}: runtime::records_rust::imports: exit
2022-10-12T17:50:48.400939Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="swap-tuple"}: runtime::records_rust::imports: close time.busy=56.0µs time.idle=114µs
2022-10-12T17:50:48.401089Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="roundtrip-flags1"}: runtime::records_rust::imports: new
2022-10-12T17:50:48.401150Z TRACE wit-bindgen import{module="exports" function="test-imports"}:wit-bindgen export{module="imports" function="roundtrip-flags1"}: runtime::records_rust::imports: enter
...snip
|
Filed bytecodealliance/wasmtime#5050 to capture the import func param ownership issue. |
…or now * to fully test this behavior, we need to split out the two roundtrip functions into one per argument-type: 10 total. * js & wasmtime-py hosts don't actually check alignment of arguments, so this test expects different behavior on different host architectures. this doesn't work with our testing strategy, so this should be fixed up once js & wasmtime-py check alignment.
pchickey
force-pushed
the
host-wasmtime-rust
branch
from
63e3a78 to
d65c215
Compare
|
bytecodealliance/wasmtime#5051 fixed the problems that cropped up when merging with main |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I rewrote the
wit-bindgen-gen-host-wasmtime-rustcrate to emit code which executes components, using thewasmtime::component::runtime. Previously, this crate emitted code which executed canonical ABI core modules, using the "core" wasmtime runtime.There's not a ton to say about the rewrite. The tests show that it works.
test/runtime/host.rshad some minor trimming to get rid of old concepts that don't exist anymore: export structs arent part of the store context, this was required for handles iirc.#[component]attr andComponentType, Lift, Lowerderivations down intogen-rust-libbehind a flag. This is a bit of a hack, but reduces code duplication.Ownership
I departed from the previous conventions in this crate to follow the behaviors available from the Wasmtime component runtime:
AllBorrowedstyle (e.g.&[&str]) parameters, and return owned (e.g.Vec<String>) results.When it becomes possible to pass imports any sort of borrowed parameter, we can make that improvement to this crate. This may end up saving a copy.
No more
host-wasmtime-rustAll significant code in the
wit-bindgen-host-wasmtime-rustcrate was a "runtime" for the previous implementation of components on top of core modules. This is all replaced by the Wasmtime implementation, so I was able to turn this code into a slim re-export of the proc macros, and the crate dependencies used by the proc macros (pub use {anyhow, wasmtime, tracing};).Wasmtime dependency
This PR requires a dependency on Wasmtime's
mainbranch, at least until the 3.0.0 release is available. In particular, it requires the latestwasmparserbytecodealliance/wasmtime#5010, and these fixes to thewasmtime::component::flags!macro bytecodealliance/wasmtime#5030Tests
In the process, I had to fix a bunch of tests:
many_argumentsto only use 16 arguments (from 20), because that is the (arbitrary) limit on the TypedFunc argument tuple size.unionto have a host wasmtime rust implementationinvalidto reflect spec changes that out of bounds integers are truncated. I moved some testing of unaligned structures to this test as well, to assert that they trap.host.pytest harness forunions, but they are corrected now at any rate.Previously, the linker made WASI preview 1 imports available to canonical ABI core modules. Instead, we now use the
crates/wasi_preview_1adapter, which will trap on calls tofd_writeand other effectful functions. I did end up filling in trivial non-trapping implementations for{environ,args}_{get,sizes_get}because various guests call those as part of initialization.