-
Notifications
You must be signed in to change notification settings - Fork 97
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
experiment: custom RTS functions #4438
base: master
Are you sure you want to change the base?
Conversation
…n/ffi-custom-section
…n/ffi-custom-section
…n/ffi-custom-section
…ffi-custom-section
…ffi-custom-section
…custom-rts-utils
@@ -0,0 +1,326 @@ | |||
// Custom RTS function utilities | |||
|
|||
use alloc::vec::Vec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would potentially involve dynamic allocation using the Rust-interop EmphermalAllocator
in allocator.rs
allocating Blob
behind the scenes. Did you check how many implicit allocations happen? I wonder whether we could possibly avoid the Vec
and use custom convenience wrapper around Blob
and Array
for implementing FFI functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can definitely do this using structs which implement the FromValue
and IntoValue
traits. I'll add a partial implementation for Blob
and Array<T>
to give an initial starting point.
fn u16_from_nat16(value: Value) -> u16; | ||
fn u32_from_nat32(value: Value) -> u32; | ||
fn u64_from_nat64(value: Value) -> u64; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, we could also add float
conversions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'char' would be useful too.
(0..len) | ||
.into_iter() | ||
.map(|i| T::from_value(array.get(i), mem)) | ||
.collect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether a dynamic allocation here happens via our EmphemeralAllocator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Do we want to prevent that or just live with the unintended consequences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't define this trait, do we steer people into avoiding arguably dubious examples like array_concat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced this with two examples named blob_alloc_fast
and blob_alloc_slow
. I agree that this example should clearly indicate that this is slower than directly working in the Motoko values themselves.
rts/motoko-rts/src/custom.rs
Outdated
let dest = array.payload_addr(); | ||
$(*(dest.add($index)) = $name::into_value(self.$index, mem)?;)+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be (not sure whether it compiles, better double check):
let dest = array.payload_addr(); | |
$(*(dest.add($index)) = $name::into_value(self.$index, mem)?;)+ | |
$(array.initialize($index, $name::into_value(self.$index, mem)?, mem);)+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That worked; thanks! Also updated everywhere else that was allocating an array.
Feel free to resolve this discussion if everything looks right.
}; | ||
} | ||
|
||
// Temporary examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure on what to ship as inbuilt examples. Some cases look general, some less (e.g. manual_alloc
). Maybe we could restrict the predefined set to general cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to move these out of this file and into test cases rather than including these in the RTS by default. This is next on the priority list now that we're planning to potentially merge this PR.
|
||
export_fun env "i8_from_int8" ( | ||
Func.of_body env ["v", I32Type] [I32Type] (fun env -> | ||
G.i (LocalGet (nr 0l)) ^^ TaggedSmallWord.lsb_adjust Type.Int8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, TaggedSmallWord.untag
should be applied too before lsb_adjust
, e.g. to sanity check that the tags are correct. Analogously for the subsequent conversions.
src/codegen/compile.ml
Outdated
(* TODO: type checking error *) | ||
Printf.printf "%s" Diag.(string_of_message { | ||
sev = Error; | ||
code = "M0199"; | ||
at; | ||
cat = "RTS"; | ||
text = Printf.sprintf "custom function '%s' not found" s'; | ||
}); | ||
exit 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the TODO, raising an error would be the nicer way. This would need some handling by the compiler pipeline, e.g. similar to errors raised during the linker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very much agreed. I investigated this with @crusso earlier this week, and it seems like this may require significant refactoring to get this to work (at least as part of the type checking phase).
Using the pipeline error handling would also be important for moc.js if someone wanted to compile Motoko programs with a custom RTS in the browser.
.collect(); | ||
|
||
// Motoko return value | ||
let ret = quote!(crate::types::Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether all FFI functions would have a Value
return type. Maybe there are some functions without return type (e.g. that implement checks with traps or do some updates).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had IntoArgs
and FromArgs
traits to account unit and tuple return types, but it turns out that the current implementation actually works better in these cases due to how the RTS functions add return values to the stack. We could potentially optimize this to use a variable number of Wasm function outputs, although I'm seeing this as probably out of scope for this PR.
Very nice PR, Ryan. Thanks a lot. This offers a well-structured, convenient small framework for FFI implementations. As you say, it would still require advanced knowledge to implement FFI. (Especially, also the GC aspects, e.g. keep Rust pointers only temporarily, applying the right GC barriers etc.). |
PS: I believe we could add some more tests for the FFI. I could also do some more stress testing with the GC, e.g. composing an additional GC random test or benchmark case that makes use of FFI. |
rts/motoko-rts/src/custom.rs
Outdated
TAG_BLOB => { | ||
let blob = value.as_blob(); | ||
let len = blob.len().as_u32(); | ||
Ok(BlobVec((0..len).into_iter().map(|i| blob.get(i)).collect())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably also hiding uses of the ephemeral allocator. I assume collect is allocating a Vec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's correct. This is equivalent to something along the lines of let vec = Vec::with_capacity(len); for i in 0..len { ... }; vec
.
This is a really good point @luc-blaeser. Because safety is a key part of Motoko's brand, this by itself makes a fairly strong case for developers to avoid this FFI approach when possible. One possibility could be to repurpose this PR as an internal refactor (implementing the built-in RTS functions using the While this functionality is currently opt-in via the |
This PR adds utility macros, traits, and functions which can be used to implement Rust FFI bindings in the RTS.
I'm currently focusing on just a few Motoko types (primarily
Blob
,Array
, tuples, and numeric primitives). It's relatively simple to expand support by implementingFromValue
andIntoValue
for other Rust types.Rust functions (included in PR for now):
Motoko usage (
ffi.mo
):Try this yourself (with placeholders
ffi.mo
and../motoko-base
):MOC_UNLOCK_PRIM=1 moc -c ffi.mo -wasi-system-api --package base ../motoko-base/src && wasmtime ffi.wasm
Changes:
rts_sections
in Wasm module decodercustom_rts_functions
field in compilation environment"rts:*"
primitive functions which refer to names in the custom sectionFromValue
andIntoValue
traits in RTS#[motoko]
procedural macro attribute which wraps#[ic_mem_fn]
and generates a custom sectionproc-macro2
andsyn
in themotoko-rts-macros
crate#[motoko]
attributeFromValue
andIntoValue
for tuples"rts:*"
primitive expressions?