Skip to content
This repository has been archived by the owner on Aug 14, 2023. It is now read-only.

Wasmer 1.0.0 alpha5 #187

Merged
merged 35 commits into from
Nov 25, 2020
Merged

Wasmer 1.0.0 alpha5 #187

merged 35 commits into from
Nov 25, 2020

Conversation

YaronWittenstein
Copy link
Contributor

Motivation

  • Updating wasmer to version 1.0.0-alpha5
  • Changing the way external imports (i.e FFI imports) work according to the new wasmer c-api

@YaronWittenstein YaronWittenstein self-assigned this Nov 17, 2020
@YaronWittenstein YaronWittenstein added the must-for-mainnet Must be done before Mainnet label Nov 17, 2020
@YaronWittenstein YaronWittenstein marked this pull request as ready for review November 18, 2020 15:15
crates/svm-runtime-c-api/tests/api_tests.rs Outdated Show resolved Hide resolved
#[derive(Clone)]
#[repr(C)]
pub struct svm_env_t {
pub inner_env: *const c_void,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since inner_env is always a Context, any reason for not calling it ctx?

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's a matter of taste. I don't really mind changing that.
The motivation was to keep the API with the env naming.


if let Some(callback) = callback {
let args = prepare_args(args);
let ctx = env.inner_mut();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can settle on having a single name, env or ctx, in all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code base still uses inner_env under svm_env_t
If you feel it's critical let's chat about that

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine, whatever you think.

unsafe extern "C" fn trampoline(
env: *mut c_void,
args: *const wasm_val_vec_t,
results: *mut wasm_val_vec_t,
Copy link
Contributor

Choose a reason for hiding this comment

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

The usage of wasm_val_vec_t (in wasm_func_callback_with_env_t signature) creates a dependency between svm client and wasmer's C API.
This type is being constructed after wasmer is calling svm's inner callback, hence svm could use a different type and a different signature for trampoline.

This type is defined in wasmer using a macro. I'm not sure if importing it will require merely using the C headers, or the lib as well. In any case, this would be suboptimal.

It seems the most straightforward to use svm's Vec<WasmValue> (which is also more restrictive in the supported Wasm types). Its encoding format is already defined and used. We can use it, or define Vec<T> and WasmValue as C types, similarly to how it's done in wasmer C API.

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 point, I've overlooked that fact (I guess I was on an auto-pilot since I'm used to the way things were in the past in the old version of wasmer)

let trap: Box<wasm_trap_t> = Box::from_raw(trap);

/// TODO: we want access to `trap.inner`
/// (this field has visibility of `pub(crate)`)
Copy link
Contributor

Choose a reason for hiding this comment

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

So we don't have a solution for this yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, we need to ask the wasmer team to change that

env: *mut c_void,
args: *const wasm_val_vec_t,
results: *mut wasm_val_vec_t,
) -> *mut wasm_trap_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

wasm_trap_t also has the same problem of creating a dependency with wasmer's C API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - same


pub host_env: *const c_void,

pub returns: svm_wasm_types_t,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this field? Will it ever need to be used by the extern client?
I see that svm use it in inner_callback to allocate the results buffer, but I don't think it's necessary - why not just read it via closure, instead of doing set/get via env?


bytes.write_u8(nvalues as u8).unwrap();

for ty in types {
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 rather have a simpler version of this function, and have it to merely allocate the needed buffer size.
But you can leave as-is if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment explaining why I prefer that we allocate the worst case.
Since memory allocation is a costly operation we should strive to minimize the number of its usages.

The savings of a few bytes is negligible in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my comment in #187 (comment). In this comment I'm not referring to that, but to the filling of the allocated bytes (with nvalues, and ty for each value). Although the extern client must comply with the signature, I think it would be better for this function to just allocate the buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

crates/svm-ffi/src/value.rs Outdated Show resolved Hide resolved
#[allow(non_camel_case_types)]
#[derive(Clone)]
#[repr(C)]
pub struct svm_byte_array {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be exposed via the C API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - I'm on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only workaround I've found for now is:

  1. use cbindgen for the svm-ffi crate - it should output a C header named svm_types.h
    This header file will contain the definition for svm_byte_array along with other used FFI-structs within svm-runtime-c-api

  2. The cbindgen of svm-runtime-c-api will still output the svm.h header but it will have an include statement of include "svm_types.h" inside.

  3. Each successful GitHub Actions CI run will have svm_types.h in its generated artifacts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the artifacts - works fine.

// this heap-allocated memory will be released by SVM.
// (See: `ExternImport#wasmer_export`)

Box::into_raw(Box::new(trap))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previously with results, this is expected to be allocated by the extern client, and to be released by svm, hence can lead to a crash or a leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a bit tricky since SVM can't know ahead-of-time how much space to allocate for svm_trap_t.
The only workaround I've in mind is to agree on a maximum trap size - say 1024 bytes for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

In go-wasmer the solution is:

  1. Allocate the error in Go
  2. Create C-compatible alias that Rust can use (via to_wasm_trap_new, defined in cgo preamble)
  3. Call Rust's C-API and re-allocate it on the heap (via wasm_trap_new)
  4. Go allocation to be automatically released
  5. Rust allocation to be released by Go (via wasm_trap_delete). I'm not sure about this step, because Rust is using the trap raw point (trampoline return value), so it's not clear how would it still be valid (if released by Go immediately after trampoline returns), or why Go even need to bother with timing its release. So i'm either missing something, or the code is still in the works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

svm-runtime-c-api added a method of trap allocation. (also a method for deallocation - in general, it should be used only for tests since SVM is in charge of deallocation of traps)

#[allow(non_camel_case_types)]
#[derive(Clone)]
#[repr(C)]
pub struct svm_byte_array {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only workaround I've found for now is:

  1. use cbindgen for the svm-ffi crate - it should output a C header named svm_types.h
    This header file will contain the definition for svm_byte_array along with other used FFI-structs within svm-runtime-c-api

  2. The cbindgen of svm-runtime-c-api will still output the svm.h header but it will have an include statement of include "svm_types.h" inside.

  3. Each successful GitHub Actions CI run will have svm_types.h in its generated artifacts.


bytes.write_u8(nvalues as u8).unwrap();

for ty in types {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment explaining why I prefer that we allocate the worst case.
Since memory allocation is a costly operation we should strive to minimize the number of its usages.

The savings of a few bytes is negligible in this case.

// this heap-allocated memory will be released by SVM.
// (See: `ExternImport#wasmer_export`)

Box::into_raw(Box::new(trap))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a bit tricky since SVM can't know ahead-of-time how much space to allocate for svm_trap_t.
The only workaround I've in mind is to agree on a maximum trap size - say 1024 bytes for example.


if let Some(callback) = callback {
let args = prepare_args(args);
let ctx = env.inner_mut();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code base still uses inner_env under svm_env_t
If you feel it's critical let's chat about that

pub func_ptr: *const c_void,
params: Vec<WasmType>,

returns: Rc<Vec<WasmType>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moshababo
I've thought about how to simplify the code of the results allocations and still gain performance.

So I've picked using Rc to share the returns types with the inner_callback using shallow clone.
(the runtime overhead is minimal and should be much less expansive than cloning the Vec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good.


match Vec::<WasmValue>::try_from(&results) {
Ok(vals) => {
// TODO: validate the returns types.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, this should be done via the host trampoline as part of injecting the results.

I'm not sure we want to have this validation logic duplicated here

Copy link
Contributor

@moshababo moshababo Nov 23, 2020

Choose a reason for hiding this comment

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

Yes, we can probably avoid doing it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added validation of the results since SVM only allocates a zeroed-buffer before calling the host function.

.include_item("svm_trap_t")
.include_item("svm_func_callback_t")
.include_item("svm_env_t")
.with_documentation(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why without documentation (unlike svm-runtime-c-api)?

_ => panic!("Only i32 and i64 are supported."),
})
.collect()
}

#[inline]
fn to_wasm_values(bytes: &svm_byte_array, types: &[WasmType]) -> Option<Vec<WasmValue>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to return Result instead of Option, since you later transform the None option to Err anyway? That way you could return more specific errors from to_wasm_values.


const COUNTER_MUL_FN_INDEX: u32 = 123;

fn wasm_trap(err: String) -> *mut svm_trap_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for still keeping this function?

// that there no trap has occurred.
return std::ptr::null_mut();
}
Err(err) => wasm_trap(err.to_string()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of wasm_trap you should use the new svm_trap_alloc C-API function.


/// Allocates a new `svm_trap_t` with inner `error` of size `size`.
#[no_mangle]
pub unsafe extern "C" fn svm_trap_alloc(size: u32) -> *mut svm_trap_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that a simpler design would be to accept svm_byte_array msg and to re-allocate it on the heap using Rust (similarly to wasmer's wasm_trap_new). Copying bytes to an unsafe buffer should work in Go, but it's quite hacky.

Vec::<WasmValue>::try_from(args).map_err(|_| "Invalid args")
}

fn wasm_error(msg: String) -> *mut svm_byte_array {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for still using this instead of svm_wasm_error_create?

@YaronWittenstein YaronWittenstein merged commit face038 into master Nov 25, 2020
@YaronWittenstein YaronWittenstein deleted the wasmer-1.0.0-alpha5 branch November 25, 2020 16:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
must-for-mainnet Must be done before Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants