🎁 Update to wasmtime 22.0.0 #392
Conversation
lib/src/async_io.rs
Outdated
| // TODO: wiggle only supports slices of u8 in 22.0.0 | ||
| .to_vec(handles)? | ||
| .into_iter() | ||
| .map(|i| AsyncItemHandle::from(i).into()), |
There was a problem hiding this comment.
Is there a limitation implied here, like we can only support 256 async item handles rather than the full u32 range of handles previously supported?
There was a problem hiding this comment.
OH, is the limitation that we can only have slices of u8s, but we can have full Vecs of other scalar types?
There was a problem hiding this comment.
Yep, that's exactly it. I'm going to fix wiggle so that we can have slices of other things, and this comment was mostly a reminder to me that this should be updated to not copy in the future.
There was a problem hiding this comment.
There's quite a few lines in this diff, but they're pleasantly straightforward and mechanical. I had feared this would be a more disruptive transition than it actually turned out to be, and I'm really happy to have more of the ownership properties now checked statically.
The only thing I might be tempted to change here is the comment in the code about only supporting slices of u8. It's a bit ambiguous to the reader, and it might cause some surprise until they figure out that we're using a Vec instead of a slice rather than a u8 instead of some other type.
There was a problem hiding this comment.
I think there are two comments that could use at least a bit more explanation, but otherwise this looks good. I tried to be pretty careful in looking at the diffs, but I have to admit that my eyes starting glazing over at some points. I can confirm that the tests still pass locally, though.
lib/src/async_io.rs
Outdated
| .iter() | ||
| .copied() | ||
| memory | ||
| // TODO: wiggle only supports slices of u8 in 22.0.0 |
There was a problem hiding this comment.
This comment is confusing me a bit. Based on the comment, I would guess that memory.to_vec(handles) returns a Vec<u8>, which would be a problem because that would mean we'd be creating AsyncItemHandles from bytes instead of 32-bit values. But if I look at the wiggle docs, it looks like memory_to_vec(handles) is going to keep the type T (in this case, u32), so we'll be doing the right thing here.
So ... what's up?
There was a problem hiding this comment.
Yep, this needs to be reworded. The issue is that the as_slice function on GuestMemory will only handle GuestPtr<[u8]>, and not GuestPtr<[T]>. This was just an oversight, and in the future we can avoid copying with to_vec here.
There was a problem hiding this comment.
(Just to be really clear: this does the right thing when producing a vector of u32, but it copies in the process)
lib/src/wiggle_abi/req_impl.rs
Outdated
| .iter() | ||
| .map(|handle| PendingRequestHandle::from(*handle).into()), | ||
| memory | ||
| // TODO: wiggle only supports slices of u8 in 22.0.0 |
|
I've updated the comment to hopefully clarify the situation with the use of |
elliottt
force-pushed
the
trevor/wasmtime-22
branch
from
33e9acc
to
39c5598
Compare
Update to wasmtime 22.0.0. This included some pretty substantial changes to the wiggle crate, which removes runtime borrow checking of guest memory. This is a huge ergonomic improvement, as the borrows are all tracked as rrust borrows instead. One piece of functionality that's missing after the wiggle changes is the ability to interact with a slice of guest memory that's not &[u8]. To work around this, I'm copying memory over to the host, but in future releases of wiggle we should have the ability to interact with slices that aren't just to u8.
Update to wasmtime 22.0.0.
This included some pretty substantial changes to the wiggle crate, which removes
runtime borrow checking of guest memory. This is a huge ergonomic improvement,
as the borrows are all tracked as rrust borrows instead.
One piece of functionality that's missing after the wiggle changes is the
ability to interact with a slice of guest memory that's not
&[u8]. To workaround this, I'm copying memory over to the host, but in future releases of
wiggle we should have the ability to interact with slices that aren't just to
u8.