Update to wasmtime v19 #322

saulecabrera · 2024-05-05T20:56:24Z

This commit reworks the bindings to support wasmtime v19 which introduces significant changes regarding reference types, namely ValType adds support for typed function refereces and GC. It's important to note that given that these features are not enabled by default, these proposals are not visible in wasmtime-rb's API surface. It's reasonable to add support for them once the implementation in Wasmtime proper is enabled by default, however, this commit paves the road in a couple of places to support the Ref type.

Full changelog: https://github.com/bytecodealliance/wasmtime/blob/main/RELEASES.md#1900

saulecabrera · 2024-05-05T21:04:28Z

ext/src/ruby_api/convert.rs

@@ -122,22 +127,42 @@ impl ToExtern for Value {
 }

 pub trait ToSym {
-    fn to_sym(self) -> Symbol;
+    fn as_sym(&self) -> Result<Symbol, Error>;


ValType is no longer Copy, which is the main motivation for this change. I also suspect that once APIs for typed function reference and GC are introduced it's likely that we will have to rework the relationship between ValType and its Ruby representation.

What's your rationale for the naming change?

#to_sym is the normal name for converting to a Symbol, so I think the name could stay the same even if it can now raise.

The name change was mostly to reflect owned -> borrow in the signature, but then I realized that I misinterpreted this table https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

Reverted!

saulecabrera · 2024-05-05T21:06:27Z

ext/src/ruby_api/params.rs

@@ -10,9 +10,6 @@ struct Param {
    index: u32,
    ty: ValType,
 }
-// Keep `Param` small so copying it to the stack is cheap, typically anything
-// less than 3usize is good
-assert_eq_size!(Param, [u64; 2]);


ValType is not Copy anymore, so I don't think this assert is useful anymore, but let me know if there's anything that I'm missing here cc @ianks

It may still have a small performance impact (we can benchmark it) but if that's the case we can come back to optimize it, it doesn't sound like a big deal?

I don't think it's a big deal to be honest, but we can always come back later if this ends up being an issue.

What's the new size of it? Can we assert on the new size instead to avoid accidentally bloating it?

saulecabrera · 2024-05-06T14:11:43Z

macos-latest, 3.1 is reporting a dynamic linking error

dyld[10102]: missing symbol called

I'm not entirely sure what's going on, however, the symptoms look very similar to oxidize-rb/rb-sys#157

jbourassa · 2024-05-07T14:09:50Z

ext/src/ruby_api/convert.rs

@@ -122,22 +127,42 @@ impl ToExtern for Value {
 }

 pub trait ToSym {
-    fn to_sym(self) -> Symbol;
+    fn as_sym(&self) -> Result<Symbol, Error>;


What's your rationale for the naming change?

#to_sym is the normal name for converting to a Symbol, so I think the name could stay the same even if it can now raise.

jbourassa · 2024-05-07T14:14:44Z

ext/src/ruby_api/convert.rs

+        if ty.matches(&ValType::EXTERNREF) {
+            let extern_ref_value = match self.is_nil() {
+                true => None,
+                false => Some(ExternRef::new(ExternRefValue::from(*self))),
+            };
+
+            return Ok(Val::ExternRef(extern_ref_value));
+        }
+
+        if ty.matches(&ValType::FUNCREF) {
+            let func_ref_value = match self.is_nil() {
+                true => None,
+                false => Some(*<&Func>::try_convert(*self)?.inner()),
+            };
+            return Ok(Val::FuncRef(func_ref_value));
+        }
+
        match ty {
            ValType::I32 => Ok(i32::try_convert(*self)?.into()),
            ValType::I64 => Ok(i64::try_convert(*self)?.into()),
            ValType::F32 => Ok(f32::try_convert(*self)?.into()),
            ValType::F64 => Ok(f64::try_convert(*self)?.into()),
-            ValType::ExternRef => {
-                let extern_ref_value = match self.is_nil() {
-                    true => None,
-                    false => Some(ExternRef::new(ExternRefValue::from(*self))),
-                };
-
-                Ok(Val::ExternRef(extern_ref_value))
-            }
-            ValType::FuncRef => {
-                let func_ref_value = match self.is_nil() {
-                    true => None,
-                    false => Some(*<&Func>::try_convert(*self)?.inner()),
-                };
-                Ok(Val::FuncRef(func_ref_value))
-            }
            ValType::V128 => err!("converting from Ruby to v128 not supported"),
+            // TODO: to be filled in once typed function references and/or GC
+            // are enabled by default.
+            t => err!("unsupported type: {t:?}"),


Nit / suggestion: I think it reads nicely when the RefType is a nested match:

impl ToWasmVal for Value { fn to_wasm_val(&self, ty: ValType) -> Result<Val, Error> { match ty { ValType::I32 => Ok(i32::try_convert(*self)?.into()), ValType::I64 => Ok(i64::try_convert(*self)?.into()), ValType::F32 => Ok(f32::try_convert(*self)?.into()), ValType::F64 => Ok(f64::try_convert(*self)?.into()), ValType::V128 => err!("converting from Ruby to v128 not supported"), ValType::Ref(ty) => { if ty.matches(&RefType::EXTERNREF) { let extern_ref_value = match self.is_nil() { true => None, false => Some(ExternRef::new(ExternRefValue::from(*self))), }; Ok(Val::ExternRef(extern_ref_value)) } else if ty.matches(&RefType::FUNCREF) { let func_ref_value = match self.is_nil() { true => None, false => Some(*<&Func>::try_convert(*self)?.inner()), }; Ok(Val::FuncRef(func_ref_value)) } else { // TODO: to be filled in once typed function references and/or GC // are enabled by default. err!("unsupported type: {ty:?}") } } } } }

Same below. Use this version if you like it too, or disregard if you don't!

ext/src/ruby_api/func.rs

jbourassa · 2024-05-07T14:43:57Z

ext/src/ruby_api/params.rs

@@ -10,9 +10,6 @@ struct Param {
    index: u32,
    ty: ValType,
 }
-// Keep `Param` small so copying it to the stack is cheap, typically anything
-// less than 3usize is good
-assert_eq_size!(Param, [u64; 2]);


It may still have a small performance impact (we can benchmark it) but if that's the case we can come back to optimize it, it doesn't sound like a big deal?

ianks · 2024-05-09T21:45:00Z

You can disable the rust-crate example on ruby 3.1, no idea what's going on there and why it's now failing 🤔

This commit reworks the bindings to support `wasmtime` 19 which introduces significant changes regarding reference types, namely `ValType` adds support for typed function refereces and GC. It's important to note that given that these features are not enabled by default, these proposals are not visible in `wasmtime-rb`'s API surface. It's reasonable to add support for them once the implementation in Wasmtime proper is enabled by default, however, this commit paves the road in a couple of places to support the `Ref` type.

Errors in cross store args/returns have changed, this commit updates the test expectations to match those changes and also fixes a multiple dynamic borros bug.

Updates the gem's version

saulecabrera commented May 5, 2024

View reviewed changes

saulecabrera requested review from ianks and jbourassa May 5, 2024 21:06

jbourassa approved these changes May 7, 2024

View reviewed changes

saulecabrera force-pushed the update-to-wasmtime-v19 branch from 6ba3e37 to 6f73683 Compare May 7, 2024 16:57

ianks approved these changes May 9, 2024

View reviewed changes

saulecabrera force-pushed the update-to-wasmtime-v19 branch from c74ab47 to b9b2a67 Compare May 10, 2024 20:28

saulecabrera added 6 commits May 10, 2024 16:29

Handle linker borrows and update func spec error messages

2137ed8

Errors in cross store args/returns have changed, this commit updates the test expectations to match those changes and also fixes a multiple dynamic borros bug.

Remove unused type entry

70fac0f

Update version.rb

92ade08

Updates the gem's version

Review comments

a358505

Disable examples in macos-latest, 3.1

7c8f7f6

saulecabrera force-pushed the update-to-wasmtime-v19 branch from b9b2a67 to 7c8f7f6 Compare May 10, 2024 20:30

saulecabrera mentioned this pull request May 10, 2024

Add ability to configure allocation_strategy #323

Merged

saulecabrera merged commit ffd785c into main May 10, 2024
23 checks passed

saulecabrera deleted the update-to-wasmtime-v19 branch May 10, 2024 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to wasmtime v19 #322

Update to wasmtime v19 #322

saulecabrera commented May 5, 2024 •

edited

Loading

saulecabrera May 5, 2024

jbourassa May 7, 2024

saulecabrera May 7, 2024

saulecabrera May 5, 2024

jbourassa May 7, 2024

saulecabrera May 7, 2024

ianks May 8, 2024

saulecabrera commented May 6, 2024

jbourassa May 7, 2024

jbourassa May 7, 2024

jbourassa May 7, 2024

ianks commented May 9, 2024

Update to wasmtime v19 #322

Update to wasmtime v19 #322

Conversation

saulecabrera commented May 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saulecabrera commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianks commented May 9, 2024

saulecabrera commented May 5, 2024 •

edited

Loading