Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/wit #31

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

MarkintoshZ
Copy link
Contributor

Use WIT definition for importing host functions as per lunatic-solutions/lunatic#86 (comment)

This is an initial attempt at using wit-bindgen crate to generate wasm function imports. Currently, I only have experimented with using wit-bindgen on the rust-lib and it has passed all the unit tests.

There are several design changes/sacrifice that was made to the VM and rust-lib to get this to work:

  • In .wit files, identifiers are strictly in kebab-case. In order to comply with wit-bindgen, I had to rename the module name and the function name.
  • Using wit-codegen, the imported functions are not marked as unsafe. I removed all the unsafe blocks for the host function calls since the compiler is giving me unnecessary unsafe block warning. I am unsure if there are negative implications of removing them that I am not aware of.

I think we might be cautious about depending on .wit files at this time because according to the readme of wit-bindgen, the library is not yet stable and the .wit format does not have a formal definition yet (bytecodealliance/wit-bindgen#251 (comment)).

Those are some of my thoughts and I would love some feedback.

@MarkintoshZ MarkintoshZ changed the base branch from main to distributed_v2 June 19, 2022 14:29
@MarkintoshZ MarkintoshZ changed the base branch from distributed_v2 to main June 19, 2022 14:29
@bkolobara
Copy link
Contributor

Thanks @MarkintoshZ for taking the initiative with this one. I'm curious how the wit-bindgen macro affects compile-times? I assume a lot of code needs to be parsed and generated and it may take some time to do so.

One way forward for us could be to define the .wit files, but still hand-implement them following the Canonical ABI on both host and guest sides. In some way we already follow the Canonical ABI for most of the stuff. The only difference is that lunatic's host resources are not always Clone and the Canonical ABI expects them to be: bytecodealliance/wit-bindgen#148

However, I think @withtypes is changing already how resources are handled in lunatic with the new distribute lunatic system and maybe we can re-design this part to better fit what .wit expects.

@MarkintoshZ
Copy link
Contributor Author

This is the build timing graph after running cargo build --timings. Looks like the bindgen macros are being compiled in parallel with other crates, and are not adding much to the total compile time if any. My CPU usage also did not go up to 100% during build on my Core i7 MacBook.
image

So should we put the task of adopting .wit on pause until the completion of the distributed lunatic system?

@withtypes
Copy link
Contributor

@MarkintoshZ let's see next week. The distributed branch doesn't have a lot of changes in the host functions, only a new namespace with a few additional host functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants