Skip to content
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

Synchronize mutable env in functions #1625

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## **[Unreleased]**

- [#1625](https://github.com/wasmerio/wasmer/pull/1625) Synchronize mutable Env access in function calls.

## 1.0.0-alpha3 - 2020-09-14
- [#1620](https://github.com/wasmerio/wasmer/pull/1620) Fix bug causing the Wapm binary to not be packaged with the release
- [#1619](https://github.com/wasmerio/wasmer/pull/1619) Improve error message in engine-native when C compiler is missing
Expand Down
27 changes: 17 additions & 10 deletions lib/api/src/externals/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ use crate::FunctionType;
use crate::NativeFunc;
use crate::RuntimeError;
pub use inner::{FromToNativeWasmType, HostFunction, WasmTypeList, WithEnv, WithoutEnv};
use std::cell::RefCell;
use std::borrow::BorrowMut;
use std::cmp::max;
use std::fmt;
use std::sync::Mutex;
use wasmer_vm::{
raise_user_trap, resume_panic, wasmer_call_trampoline, Export, ExportFunction,
VMCallerCheckedAnyfunc, VMContext, VMDynamicFunctionContext, VMFunctionBody, VMFunctionKind,
Expand Down Expand Up @@ -126,7 +127,7 @@ impl Function {
Env: Sized + 'static,
{
let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithEnv {
env: RefCell::new(env),
env: Mutex::new(Box::new(env)),
func: Box::new(func),
function_type: ty.clone(),
});
Expand Down Expand Up @@ -226,7 +227,7 @@ impl Function {
// Wasm-defined functions have a `VMContext`.
// In the case of Host-defined functions `VMContext` is whatever environment
// the user want to attach to the function.
let box_env = Box::new(env);
let box_env = Box::new(Mutex::new(env));
let vmctx = Box::into_raw(box_env) as *mut _ as *mut VMContext;
let signature = function.ty();

Expand Down Expand Up @@ -409,7 +410,6 @@ impl Function {
)));
}
}

Ok(NativeFunc::new(
self.store.clone(),
self.exported.address,
Expand Down Expand Up @@ -463,24 +463,27 @@ impl VMDynamicFunction for VMDynamicFunctionWithoutEnv {
}
}

#[repr(C)]
pub(crate) struct VMDynamicFunctionWithEnv<Env>
where
Env: Sized + 'static,
{
function_type: FunctionType,
#[allow(clippy::type_complexity)]
func: Box<dyn Fn(&mut Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
env: RefCell<Env>,
// We have to box this because of the way we call the `call` function, we don't know the
// size of the env when calling from NativeFunc.
env: Mutex<Box<Env>>,
}

impl<Env> VMDynamicFunction for VMDynamicFunctionWithEnv<Env>
where
Env: Sized + 'static,
{
fn call(&self, args: &[Val]) -> Result<Vec<Val>, RuntimeError> {
// TODO: the `&mut *self.env.as_ptr()` is likely invoking some "mild"
// undefined behavior due to how it's used in the static fn call
unsafe { (*self.func)(&mut *self.env.as_ptr(), &args) }
let mut env_guard = self.env.lock().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really a fan of this. This is a clear performance penalty that we are now incurring for all function calls.

let env_mut = env_guard.borrow_mut();
(*self.func)(&mut **env_mut, &args)
}
fn function_type(&self) -> &FunctionType {
&self.function_type
Expand Down Expand Up @@ -1022,18 +1025,22 @@ mod inner {
/// This is a function that wraps the real host
/// function. Its address will be used inside the
/// runtime.
extern fn func_wrapper<$( $x, )* Rets, RetsAsResult, Env, Func>( env: &mut Env, $( $x: $x::Native, )* ) -> Rets::CStruct
extern fn func_wrapper<$( $x, )* Rets, RetsAsResult, Env, Func>( env: &std::sync::Mutex<Env>, $( $x: $x::Native, )* ) -> Rets::CStruct
where
$( $x: FromToNativeWasmType, )*
Rets: WasmTypeList,
RetsAsResult: IntoResult<Rets>,
Env: Sized,
Func: Fn(&mut Env, $( $x ),* ) -> RetsAsResult + 'static
{
use std::borrow::BorrowMut;

let func: &Func = unsafe { &*(&() as *const () as *const Func) };
let mut env_guard = env.lock().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really a fan of this. This is a clear performance penalty that we are now incurring for all function calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should measure this penalty, because I don't see a lot more options to solve this sync problem.

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'm completely in favor of measurement, but I'm also sympathetic to wanting a fast path for singlethreaded code which will likely be the majority of uses (at least in the short-term).

Also, I agree that yeah, this change is nice because it just works without much changes. I wrote up a design doc about solutions to this problem and I'll ping people about it next week: I think it's probable that we can make a more complex solution to this problem, but it will require breaking changes to our API or duplicating our API (singlethreaded version vs general version). I also talked about some more exotic options with Nick in regards to things like updating vtables at runtime to dynamically synchronize code when it becomes multithreaded but I don't think we're realistically going to be trying anything fancy any time soon and also I haven't thought of any fancy solutions that aren't a bad trade off in terms of complexity.

let env_mut: &mut Env = env_guard.borrow_mut();

let result = panic::catch_unwind(AssertUnwindSafe(|| {
func(env, $( FromToNativeWasmType::from_native($x) ),* ).into_result()
func(env_mut, $( FromToNativeWasmType::from_native($x) ),* ).into_result()
}));

match result {
Expand Down
2 changes: 1 addition & 1 deletion lib/api/src/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ macro_rules! impl_native_traits {
let ctx = self.vmctx as *mut VMContextWithoutEnv;
unsafe { (*ctx).ctx.call(&params_list)? }
} else {
type VMContextWithEnv = VMDynamicFunctionContext<VMDynamicFunctionWithEnv<std::ffi::c_void>>;
type VMContextWithEnv = VMDynamicFunctionContext<VMDynamicFunctionWithEnv<Box<std::ffi::c_void>>>;
let ctx = self.vmctx as *mut VMContextWithEnv;
unsafe { (*ctx).ctx.call(&params_list)? }
};
Expand Down