Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

Support async in lucet-wiggle, and switch lucet-wasi to use tokio #655

Merged
merged 16 commits into from
May 19, 2021

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented May 11, 2021

tldr

  • Lucet-wiggle is more like wasmtime-wiggle now.
  • Lucet-wasi is a lot more like wasmtime-wasi, as well.
  • We are borrowing code from the new-api wasmtime PR until it lands.
  • Lucet-wasi requires a tokio runtime now.

Support async in lucet-wiggle

Earlier this year, support for async in wiggle landed in upstream wasmtime in bytecodealliance/wasmtime#2701, and using this async functionality so that wasi-common can operate on tokio landed in bytecodealliance/wasmtime#2832.

This PR ports Lucet to use those new capabilities of wiggle and wasi-common, where the entire host function is an async fn executed on the host stack via Vmctx::block_on. This is a significant departure from the existing Lucet async style of only using Vmctx::block_on in small sections. However, it is also currently impossible to use the Wiggle module traits as defined by upstream wasi-common, because currently Wasmtime does not require async host functions to impl Send, and in fact they close over WasiCtx which is not safe to Send.

To solve this impedance mismatch, this PR vendors in the changes made to the wiggle and wasi-common family of crates by @alexcrichton in bytecodealliance/wasmtime#2897. These changes transform Wiggle module traits to take &mut self rather than &self, and simplify the structure of WasiCtx so that it does now impl Send. So, this version of upstream is compatible with Lucet, but since it is gated behind RFC11 we are vendoring it until it lands. All references to the wiggle{-} and wasi- crates in the Lucet workspace have been changed to paths in the wasmtime-new-api directory.

lucet-wiggle was previously a bit clunky due to constraints in wiggle which went away a while ago. It has been redesigned to work just like wasmtime-wiggle: it re-exports wiggle, and provides its own lucet_integration! proc macro which only defines the code which hooks up the code generated from wiggle::from_witx! to the lucet runtime. Previously lucet-wiggle shipped its own from_witx! macro which combined both of those macros into one.

That change also meant we can get rid of lucet-wasi-generate entirely! No more special proc macro or boilerplate is required for hooking up the wasi-common definitions directly as Lucet hostcalls - just a single lucet_wiggle::lucet_integration! invocation.

Switch lucet-wasi to use tokio

In bytecodealliance/wasmtime#2832 I went to lengths to make give wasmtime-wasi users the option of using plain-old synchronous Rust, or using async Rust on top of tokio. I decided not to do this on Lucet and instead mandate that lucet-wasi users use tokio. My reasons for this are:

  1. Lucet makes it harder to have multiple definitions of import functions than Wasmtime does - you'd have to select whether to use the sync or async tokio definition of a hostcall at AOT compile time via bindings.json
  2. Many production users of Lucet are already running it on top of tokio.
  3. Lucet is approaching EOL - users who need flexibility should switch to wasmtime. All of the features which make wasmtime performance meet or exceed Lucet have landed.

By switching lucet-wasi to use tokio, blocking calls (notably poll_oneoff) interact with the tokio executor correctly - a poll_oneoff waiting on a timer will be implemented by tokio's Timeout future. Other blocking syscalls will use block_in_place to cooperate with the executor. Prior to this change, running lucet-wasi on top of tokio would block the thread without the executor's cooperation, which will almost certainly result in bad behavior.

The wasmtime new-api branch constains futures to be Send, and wiggle and
wasi-common got considerable changes to fix this. Lucet futures are Send
as well, so using these versions of wiggle and wasi-common solves a
bunch of integration issues.

We expect this branch to take a little bit of time to land because it is
gated behind an RFC, so vendoring these changes allows us to ship the async
wasi-common in Lucet a lot sooner.
@pchickey pchickey changed the title port to latest wasmtime async wiggle by vendoring in new-api Support async in wiggle, and switch lucet-wasi to use tokio May 12, 2021
@pchickey pchickey changed the title Support async in wiggle, and switch lucet-wasi to use tokio Support async in lucet-wiggle, and switch lucet-wasi to use tokio May 12, 2021
@pchickey pchickey marked this pull request as ready for review May 12, 2021 19:50
@pchickey pchickey requested a review from acfoltzer May 12, 2021 19:51
@pchickey pchickey force-pushed the pch/vendor_wasmtime_new_api branch from 906fafc to 2612490 Compare May 17, 2021 23:12
@pchickey pchickey force-pushed the pch/vendor_wasmtime_new_api branch from 2612490 to 622e050 Compare May 17, 2021 23:21
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Mainly some questions about the vendored code

Comment on lines +21 to +27
"wasmtime-new-api/crates/wiggle",
"wasmtime-new-api/crates/wiggle/borrow",
"wasmtime-new-api/crates/wiggle/generate",
"wasmtime-new-api/crates/wiggle/macro",
"wasmtime-new-api/crates/wasi-common",
"wasmtime-new-api/crates/wasi-common/cap-std-sync",
"wasmtime-new-api/crates/wasi-common/tokio",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be brought in via submodule instead of directly vendored? It would make potential security updates etc easier to apply.

Also, has this vendored code been reviewed in its home context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It potentially could be, but I intend for this to be short-lived enough that it doesn't matter. I am expecting it to land the main repo in a few weeks, so I didn't think it was worthwhile to add a submodule and delete it.

Yes, I have reviewed it in its home context, but not formally signed off yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Can we leave some breadcrumbs to help apply any fixes that might be made before the formal sign-off? Like a marker of what git revision it's based off, and which branch/PR to watch over in wasmtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I've added this now.

Comment on lines +16 to +17
// this will be available to dependent crates via the DEP_WASI_COMMON_19_WASI env var:
println!("cargo:wasi={}", wasi.display());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd completely forgotten this is a thing you can do

wasmtime-new-api/crates/wasi-common/build.rs Outdated Show resolved Hide resolved
lucet-wasi/src/runtime.rs Show resolved Hide resolved
@pchickey pchickey requested a review from acfoltzer May 19, 2021 17:29
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pchickey pchickey merged commit 2ba60e5 into main May 19, 2021
@pchickey pchickey deleted the pch/vendor_wasmtime_new_api branch May 19, 2021 19:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants