Clean up host env code, add more `Send`, `Sync` bounds #1944

MarkMcCaskey · 2020-12-16T16:39:39Z

This PR contains:

Code clean up, refactored the unsafe metadata creation into a function which can probably be safe
Add Send + Sync bounds in some places where they should have been to begin with

Review

Add a short description of the the change to the CHANGELOG.md file

syrusakbary · 2020-12-16T20:59:51Z

lib/api/src/externals/function.rs

+    import_init_function_ptr: for<'a> fn(
+        &'a mut Env,
+        &'a crate::Instance,
+    ) -> Result<(), crate::HostEnvInitError>,


What is the for syntax? First time I'm seeing this!

Haha yeah, it's not super common

it's used to bind a lifetime in a scope, it's a universal qualifier (like ∀), it's just saying "for all lifetimes 'a, this thing, it's a way of constraining the type (and it's required here because it'll be inferred incorrectly)

@syrusakbary That's also because you can't write :

import_init_function_ptr: fn<'a>(&'a mut Env, &'a crate::Instance) -> …,

It's a syntax error.

@MarkMcCaskey I wonder though if there is a difference with:

fn build_export_function_metadata<'a, Env>( env: Env, import_init_function_ptr: fn(&'a mut Env, &'a crate::Instance) -> …, ) -> …

Anyway, I'm OK with this change!

Yes, obviously there is a difference. The difference with 'a on the function itself is that it will be inferred from the calling context. That's not what we want! Forget it.

syrusakbary · 2020-12-16T21:00:49Z

lib/api/src/externals/function.rs

@@ -104,7 +147,7 @@ impl Function {
    pub fn new<FT, F>(store: &Store, ty: FT, func: F) -> Self
    where
        FT: Into<FunctionType>,
-        F: Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
+        F: Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static + Send + Sync,


What it means for require a function to be Send + Sync ?
Could it still be possible to be called in parallel through different threads?

I believe Sync with Fn does mean that the function can be called from multiple threads at the same time. Send just means it can be executed on any thread.

For Send, this means functions can't use thread-local variables.

For Sync, this means functions should not use unsynchronized global variables.

I can't think of any other cases where this is really an issue right now, I'm sure there are a few other restrictions, but most functions that use unsafe correctly or don't use unsafe at all should be both Send and Sync

I think it also means that the environment captured by the function must also implement Send + Sync.

Confirmed with:

use std::rc::Rc; fn main() { fn check<F>(f: F) where F: Fn() + 'static + Send + Sync {} let x = Rc::new(42); check(|| { let _ = x; }); }

which raises the following issue:

error[E0277]: `Rc<i32>` cannot be shared between threads safely --> src/main.rs:7:5 | 4 | fn check<F>(f: F) where F: Fn() + 'static + Send + Sync {} | ---- required by this bound in `check` ... 7 | check(|| { let _ = x; }); | ^^^^^ `Rc<i32>` cannot be shared between threads safely | = help: the trait `Sync` is not implemented for `Rc<i32>` = note: required because of the requirements on the impl of `Send` for `&Rc<i32>` = note: required because it appears within the type `[closure@src/main.rs:7:11: 7:28]`

(see the playground)

@Hywan , I think having a Send + Sync environment is ultimately what we need for the Wasmer API... because Instance is Send + Sync, so a single function can be called from multiple threads concurrently and in parallel. I don't know a good way around this yet but I think Send + Sync does fix everything for us

The alternative might be to have separate code and abstractions for non-thread safe things, but that seems pretty complicated 🤔 . I think encouraging code to work on multiple threads by default is a very Rusty thing anyways and I think it positions us and users of wasmer well to scale their applications

Hywan

I like this PR. It's a very welcomed clean up!

I'm approving it to not be a blocker, but I would be delighted to see my comments addressed 🙂.

Hywan · 2020-12-18T12:48:51Z

lib/api/src/externals/function.rs

+    import_init_function_ptr: for<'a> fn(
+        &'a mut Env,
+        &'a crate::Instance,
+    ) -> Result<(), crate::HostEnvInitError>,


@syrusakbary That's also because you can't write :

import_init_function_ptr: fn<'a>(&'a mut Env, &'a crate::Instance) -> …,

It's a syntax error.

@MarkMcCaskey I wonder though if there is a difference with:

fn build_export_function_metadata<'a, Env>( env: Env, import_init_function_ptr: fn(&'a mut Env, &'a crate::Instance) -> …, ) -> …

Anyway, I'm OK with this change!

Hywan · 2020-12-18T12:54:38Z

lib/api/src/externals/function.rs

+    let import_init_function_ptr = Some(unsafe {
+        std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
+            import_init_function_ptr,
+        )
+    });


This code can be simplified to:

Suggested change

let import_init_function_ptr = Some(unsafe {

std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(

import_init_function_ptr,

)

});

let import_init_function_ptr = Some(unsafe {

std::mem::transmute::<_, fn(_, _) -> Result<(), _>>(

import_init_function_ptr,

)

});

or even:

Suggested change

let import_init_function_ptr = Some(unsafe {

std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(

import_init_function_ptr,

)

});

let import_init_function_ptr = Some(unsafe {

std::mem::transmute::<_, _>(

import_init_function_ptr,

)

});

so basically to:

Suggested change

let import_init_function_ptr = Some(unsafe {

std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(

import_init_function_ptr,

)

});

let import_init_function_ptr = Some(unsafe { std::mem::transmute(import_init_function_ptr) });

Rust is able to infer the type correctly all the way.

Yeah, that's true, I thought listing both types was good documentation though: std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>( communicates that the function arguments and error value are changing, but the number of arguments, and the fact that it's Result<(), are not. I'll rereview the code to see if this extra info is useful, but I think it's good to be super explicit with things like transmute as so many things can go wrong

lib/api/src/externals/function.rs

Hywan · 2020-12-18T13:24:11Z

lib/api/src/externals/function.rs

@@ -104,7 +147,7 @@ impl Function {
    pub fn new<FT, F>(store: &Store, ty: FT, func: F) -> Self
    where
        FT: Into<FunctionType>,
-        F: Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
+        F: Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static + Send + Sync,


I think it also means that the environment captured by the function must also implement Send + Sync.

Confirmed with:

use std::rc::Rc; fn main() { fn check<F>(f: F) where F: Fn() + 'static + Send + Sync {} let x = Rc::new(42); check(|| { let _ = x; }); }

which raises the following issue:

error[E0277]: `Rc<i32>` cannot be shared between threads safely --> src/main.rs:7:5 | 4 | fn check<F>(f: F) where F: Fn() + 'static + Send + Sync {} | ---- required by this bound in `check` ... 7 | check(|| { let _ = x; }); | ^^^^^ `Rc<i32>` cannot be shared between threads safely | = help: the trait `Sync` is not implemented for `Rc<i32>` = note: required because of the requirements on the impl of `Send` for `&Rc<i32>` = note: required because it appears within the type `[closure@src/main.rs:7:11: 7:28]`

(see the playground)

Hywan · 2020-12-18T13:26:50Z

lib/api/src/externals/function.rs

+        )
+    };
+
+    (env, metadata)


Why not just returning metadata here, and let the rest of the code accessing metadata.env to get the environment? Is it by convenience?

metadata.env is private! It can only be seen in wasmer_engine not in wasmer_API, this is for the best as the contents of the metadata struct can be dangerous

Hywan · 2020-12-18T13:28:02Z

lib/api/src/externals/function.rs

+        let import_init_function_ptr: for<'a> fn(&'a mut _, &'a _) -> Result<(), _> =
+            |env: &mut VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>>,
+             instance: &crate::Instance| {
+                Env::init_with_instance(&mut *env.ctx.env, instance)
+            };


MarkMcCaskey · 2020-12-18T15:58:32Z

bors r+

1944: Clean up host env code, add more `Send`, `Sync` bounds r=MarkMcCaskey a=MarkMcCaskey This PR contains: - Code clean up, refactored the unsafe metadata creation into a function which can probably be safe - Add `Send` + `Sync` bounds in some places where they should have been to begin with # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>

bors · 2020-12-18T16:18:24Z

Build failed:

Test on windows-x64

MarkMcCaskey · 2020-12-18T17:49:59Z

bors r+

bors · 2020-12-18T18:25:53Z

Build succeeded:

Clean up host env code, add more Send, Sync bounds

4494bea

MarkMcCaskey requested review from syrusakbary and Hywan December 16, 2020 16:39

MarkMcCaskey requested a review from nlewycky as a code owner December 16, 2020 16:39

syrusakbary reviewed Dec 16, 2020

View reviewed changes

Hywan approved these changes Dec 18, 2020

View reviewed changes

Mark McCaskey and others added 3 commits December 18, 2020 07:54

Address feedback, clean up code

fc9a8f8

Merge branch 'master' into feature/clean-up-host-env-code

e3eb529

Add 1944 to changelog

283acc5

Update deprecated API to new restrictions on dynamic envs

4f08f3d

bors bot merged commit ada9a36 into master Dec 18, 2020

bors bot deleted the feature/clean-up-host-env-code branch December 18, 2020 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up host env code, add more `Send`, `Sync` bounds #1944

Clean up host env code, add more `Send`, `Sync` bounds #1944

MarkMcCaskey commented Dec 16, 2020 •

edited

Loading

syrusakbary Dec 16, 2020

MarkMcCaskey Dec 16, 2020

Hywan Dec 18, 2020

Hywan Dec 18, 2020

syrusakbary Dec 16, 2020

MarkMcCaskey Dec 16, 2020

Hywan Dec 18, 2020

MarkMcCaskey Dec 18, 2020

MarkMcCaskey Dec 18, 2020

Hywan left a comment

Hywan Dec 18, 2020

Hywan Dec 18, 2020

MarkMcCaskey Dec 18, 2020

Hywan Dec 18, 2020

Hywan Dec 18, 2020

MarkMcCaskey Dec 18, 2020

Hywan Dec 18, 2020

MarkMcCaskey commented Dec 18, 2020

bors bot commented Dec 18, 2020

MarkMcCaskey commented Dec 18, 2020

bors bot commented Dec 18, 2020

Clean up host env code, add more Send, Sync bounds #1944

Clean up host env code, add more Send, Sync bounds #1944

Conversation

MarkMcCaskey commented Dec 16, 2020 • edited Loading

Review

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkMcCaskey commented Dec 18, 2020

bors bot commented Dec 18, 2020

MarkMcCaskey commented Dec 18, 2020

bors bot commented Dec 18, 2020

Clean up host env code, add more `Send`, `Sync` bounds #1944

Clean up host env code, add more `Send`, `Sync` bounds #1944

MarkMcCaskey commented Dec 16, 2020 •

edited

Loading