Stream: git-wasmtime

Topic: wasmtime / PR #7766 Support compilation-only build by add...

Wasmtime GitHub notifications bot (Jan 22 2024 at 09:38):

adambratschikaye edited PR #7766.

Wasmtime GitHub notifications bot (Jan 22 2024 at 09:39):

adambratschikaye has marked PR #7766 as ready for review.

Wasmtime GitHub notifications bot (Jan 22 2024 at 09:39):

adambratschikaye requested elliottt for a review on PR #7766.

Wasmtime GitHub notifications bot (Jan 22 2024 at 09:39):

adambratschikaye requested wasmtime-compiler-reviewers for a review on PR #7766.

Wasmtime GitHub notifications bot (Jan 22 2024 at 09:39):

adambratschikaye requested alexcrichton for a review on PR #7766.

Wasmtime GitHub notifications bot (Jan 22 2024 at 09:39):

adambratschikaye requested wasmtime-core-reviewers for a review on PR #7766.

Wasmtime GitHub notifications bot (Jan 22 2024 at 09:44):

adambratschikaye commented on PR #7766:

With these changes I was able to get this example of performing wasmtime compilation within wasm to work. However there was one problem with is that the compilation of wasmtime to wasm32 contains a function with ~60,000 locals and wasmparser has a hard limit of 50,000 locals per function.

So I'd like to add a test that does the compilation within wasm, but I still need to figure out how to handle that limit on locals.

Wasmtime GitHub notifications bot (Jan 22 2024 at 10:05):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:16):

alexcrichton commented on PR #7766:

That's awesome! I think you're perhaps the first person ever to execute a cross-compilation using Cranelift from a 32-bit architecture to a 64-bit architecture!

I'll dig into this today and give a review.

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:38):

alexcrichton submitted PR review:

Thanks again for this! Reading over it, everything looks at a high level ok here, but if you're ok with it I'd like to brainstorm a bit on the internal organization here. My goal here would be to cut down on "complexity" (a loose term here...) which kind of translates to minimizing #[cfg] and other things. Some possible ideas:

• Could the runtime module, at its root, have everything reexported of it's internals? My hope would be that in lib.rs we'd have:

  rust #[cfg(feature = "runtime")] mod runtime; #[cfg(feature = "runtime")] pub use runtime::*;

  I think cargo fmt has issues with modules defined via macro expansions so moving the mod runtime out of a macro may benefit that niche use case.

• I think it's ok to move some more things into runtime that are technically agnostic like limits, resources, and stack.

• Can you add a builder to CI to build the wasmtime crate for the wasm32-wasi target with the runtime feature disabled?

• Splitting engine.rs feels not great since it's now so spread out (and requires a large struct with a bunch of pub(crate) which never feels great. This might be something where it may be worth some more #[cfg]. Do you know if a design such as this would work?

  rust impl Engine { /* common methods */ } #[cfg(feature = "runtime")] impl Engine { /* runtime methods */ } #[cfg(feature = "compile")] impl Engine { /* compilation methods */ }

  that might help keep everything in a single file, albeit there'd probably be a lot of #[cfg] related to use directives at the top of the file perhaps.

• Do you think it would make sense to have a top level mod compile; with all compilation-related bits under that? Right now it's sort of spread out at the root it seems.

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:38):

alexcrichton submitted PR review:

Thanks again for this! Reading over it, everything looks at a high level ok here, but if you're ok with it I'd like to brainstorm a bit on the internal organization here. My goal here would be to cut down on "complexity" (a loose term here...) which kind of translates to minimizing #[cfg] and other things. Some possible ideas:

• Could the runtime module, at its root, have everything reexported of it's internals? My hope would be that in lib.rs we'd have:

  rust #[cfg(feature = "runtime")] mod runtime; #[cfg(feature = "runtime")] pub use runtime::*;

  I think cargo fmt has issues with modules defined via macro expansions so moving the mod runtime out of a macro may benefit that niche use case.

• I think it's ok to move some more things into runtime that are technically agnostic like limits, resources, and stack.

• Can you add a builder to CI to build the wasmtime crate for the wasm32-wasi target with the runtime feature disabled?

• Splitting engine.rs feels not great since it's now so spread out (and requires a large struct with a bunch of pub(crate) which never feels great. This might be something where it may be worth some more #[cfg]. Do you know if a design such as this would work?

  rust impl Engine { /* common methods */ } #[cfg(feature = "runtime")] impl Engine { /* runtime methods */ } #[cfg(feature = "compile")] impl Engine { /* compilation methods */ }

  that might help keep everything in a single file, albeit there'd probably be a lot of #[cfg] related to use directives at the top of the file perhaps.

• Do you think it would make sense to have a top level mod compile; with all compilation-related bits under that? Right now it's sort of spread out at the root it seems.

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:38):

alexcrichton created PR review comment:

Could you add a doc-block here and above runtime below indicating what the feature is?

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:38):

alexcrichton created PR review comment:

Mind throwing a comment above this indicating why it's here?

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:41):

alexcrichton commented on PR #7766:

So I'd like to add a test that does the compilation within wasm, but I still need to figure out how to handle that limit on locals.

Can you paste an example of buliding wasmtime like this? I checked out your repo and pointed it at this branch and this passed:

$ wasm-tools validate ./target/wasm32-wasi/release/wasmtime-in-wasm.wasm

but I expected that it might otherwise fail

Wasmtime GitHub notifications bot (Jan 23 2024 at 09:55):

adambratschikaye commented on PR #7766:

Thanks, all those comments seem reasonable and I'll look into them.

Wasmtime GitHub notifications bot (Jan 25 2024 at 13:33):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 14:27):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 14:55):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 15:12):

adambratschikaye requested wasmtime-default-reviewers for a review on PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 15:12):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 15:18):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 16:13):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 16:35):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 16:51):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 17:16):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 18:29):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):

adambratschikaye submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):

adambratschikaye created PR review comment:

Yep, added one.

Wasmtime GitHub notifications bot (Jan 25 2024 at 18:31):

adambratschikaye submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2024 at 18:31):

adambratschikaye created PR review comment:

I added one for runtime. I think this feature isn't actually necessary - I modified the cranelift build so that if all-arch is enabled it doesn't complain if it can't build for the host arch.

Wasmtime GitHub notifications bot (Jan 25 2024 at 19:05):

adambratschikaye commented on PR #7766:

Ok, I've made most of the suggested changes:

• Could the runtime module, at its root, have everything reexported of it's internals? My hope would be that in lib.rs we'd have:
  rust #[cfg(feature = "runtime")] mod runtime; #[cfg(feature = "runtime")] pub use runtime::*;
  I think cargo fmt has issues with modules defined via macro expansions so moving the mod runtime out of a macro may benefit that niche use case.

Yes this seems to be cleaner.

• I think it's ok to move some more things into runtime that are technically agnostic like limits, resources, and stack.

I moved these three as well as coredump and debug.

• Can you add a builder to CI to build the wasmtime crate for the wasm32-wasi target with the runtime feature disabled?

I added an extra build job in the CI (that runs on x86 linux). Hope this makes sense within the general CI setup of the project.

• Splitting engine.rs feels not great since it's now so spread out (and requires a large struct with a bunch of pub(crate) which never feels great. This might be something where it may be worth some more #[cfg]. Do you know if a design such as this would work?
  rust impl Engine { /* common methods */ } #[cfg(feature = "runtime")] impl Engine { /* runtime methods */ } #[cfg(feature = "compile")] impl Engine { /* compilation methods */ }
  that might help keep everything in a single file, albeit there'd probably be a lot of #[cfg] related to use directives at the top of the file perhaps.

Yep this works fine, except I didn't add a compile feature. It would just be the same as any(feature = "cranelift", feature = "winch") so I just used that everywhere.

• Do you think it would make sense to have a top level mod compile; with all compilation-related bits under that? Right now it's sort of spread out at the root it seems.

I put some things that were at the top-level under the compile module. But there's still a bunch of compilation specific stuff under cfg directives in the runtime module. Many of these seem like they wouldn't make sense to move over to the compile because they rely on types that wouldn't be used without the runtime feature. For example, Func::new requires compilation, but I don't see how it could be used without the runtime feature (there are similar examples for Linker, Module, and Component).

Wasmtime GitHub notifications bot (Jan 25 2024 at 19:19):

adambratschikaye commented on PR #7766:

Can you paste an example of buliding wasmtime like this? I checked out your repo and pointed it at this branch and this passed:

$ wasm-tools validate ./target/wasm32-wasi/release/wasmtime-in-wasm.wasm

but I expected that it might otherwise fail

Ah yep, that's working for me as well. Doing the debug build hits the limit though:

❯ cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 0.16s

❯ wasm-tools validate target/wasm32-wasi/debug/wasmtime-in-wasm.wasm
error: func 35030 failed to validate

Caused by:
    0: too many locals: locals exceed maximum (at offset 0x111420c)

Would it make sense to add that code as an example along with a CI job do the compilation in Wasm and then execute on the host system? And would you want that job to run on all hosts in the matrix?

Wasmtime GitHub notifications bot (Jan 26 2024 at 19:26):

alexcrichton submitted PR review:

Ok thanks again for you work on this, it's all looking great to me! I've got some minor nits and things but I think it's probably best to go ahead and land this and I can follow-up myself with anything else I'd like. I'd also like to get the wasmtime CLI itself compiling to wasm with just the cranelift/winch features enabled, but no need to do that here either! I think there's a rebase conflict but otherwise I'm happy to throw this at the merge queue.

Would it make sense to add that code as an example along with a CI job do the compilation in Wasm and then execute on the host system? And would you want that job to run on all hosts in the matrix?

I think so yeah, but I'll do that as part of a follow-up to get the wasmtime CLI itself compiling. I tried for a bit to figure out what function was causing issues unsuccessfully, but this is also ok to work on as a follow-up.

My guess is that it's a function in cranelift-codegen for lowering since that's the main source of "big generated code". I have yet to confirm this though.

Wasmtime GitHub notifications bot (Jan 26 2024 at 20:07):

alexcrichton commented on PR #7766:

Ah ok so I found the function:

$ wasm-tools validate ./target/wasm32-wasi/debug/wasmtime-in-wasm.wasm
error: func 39100 failed to validate

Caused by:
    0: too many locals: locals exceed maximum (at offset 0x12ebabe)
$ wasm-tools print ./target/wasm32-wasi/debug/wasmtime-in-wasm.wasm --skeleton | rg 39100
  (func $_ZN4core6option19Option$LT$$RF$T$GT$6cloned17h3ad039100bcc27f6E (;11910;) (type 2) (param i32 i32) ...)
  (func $_ZN17cranelift_codegen2ir7builder11InstBuilder8f64const17h4e1391004dd942baE (;18861;) (type 4) (param i32 i32) (result i32) ...)
  (func $_ZN17cranelift_codegen4opts14generated_code20constructor_simplify17h2e155f4b5f5d37f9E (;39100;) (type 9) (param i32 i32 i32) ...)
$ rustfilt _ZN17cranelift_codegen4opts14generated_code20constructor_simplify17h2e155f4b5f5d37f9E
cranelift_codegen::opts::generated_code::constructor_simplify

That corresponds to the ISLE-generated code for optimizing CLIF. I'm not personally aware of any low-hanging fruit on optimizing the output of ISLE for debug builds, so this may be a case where cranelift-codegen the crate needs to be optimized, even in debug builds.

Wasmtime GitHub notifications bot (Jan 29 2024 at 08:53):

adambratschikaye updated PR #7766.

Wasmtime GitHub notifications bot (Jan 29 2024 at 08:56):

adambratschikaye commented on PR #7766:

Ok thanks again for you work on this, it's all looking great to me! I've got some minor nits and things but I think it's probably best to go ahead and land this and I can follow-up myself with anything else I'd like. I'd also like to get the wasmtime CLI itself compiling to wasm with just the cranelift/winch features enabled, but no need to do that here either! I think there's a rebase conflict but otherwise I'm happy to throw this at the merge queue.

I resolved the conflicts. Happy to follow up with compiling the CLI to wasm as well.

Wasmtime GitHub notifications bot (Jan 29 2024 at 15:51):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 29 2024 at 15:52):

alexcrichton commented on PR #7766:

Ok sounds good! I don't necessarily have a concrete use case in mind but it seems like a neat way to demo how Cranelift can be compiled to wasm. I'll poke a bit more at the internals of the wasmtime crate and leave the CLI bits to you.

Wasmtime GitHub notifications bot (Jan 29 2024 at 16:27):

alexcrichton merged PR #7766.

Wasmtime GitHub notifications bot (Jan 29 2024 at 22:47):

alexcrichton commented on PR #7766:

I was curious to play around with this and went a bit far down the rabbit hole! I was able to hack things up and get things working though with https://github.com/bytecodealliance/wasmtime/pull/7842 and https://github.com/bytecodealliance/wasmtime/pull/7844, however. That's perhaps a bit of a taste of how cross-compilation, especially across pointer widths, is not the most well trodden path in Wasmtime so there may be a few other lingering bugs. Mostly just something to be aware of, and I'd hope that we can start adding regression tests in CI once we've got CLI support!

Last updated: Nov 22 2024 at 17:03 UTC