Instance should implement Sync and Send

[syrusakbary]

To be able to run Wasmer from different threads Instance will need to implement Sync and Send.

Related issue: amethyst/amethyst#1729

[syrusakbary]

We just started working on this :)

[MarkMcCaskey]

After doing more research, it seems that we don't want Instance to implement Sync but that it can be fine for it to implement Send

#[macro_use]
extern crate specs_derive;

use specs::prelude::*;
use std::borrow::BorrowMut;
use std::sync::Mutex;
use wasmer_runtime::{func, imports, instantiate, Ctx, Func, Instance, Value};
use wasmer_runtime_core::{memory::ptr::WasmPtr, types::ValueType};

#[derive(Debug, Component)]
#[storage(VecStorage)]
struct Vel(f32, f32);

#[derive(Debug, Component, Clone, Copy)]
#[storage(VecStorage)]
#[repr(C)]
struct Pos(f32, f32);

unsafe impl ValueType for Pos {}

#[derive(Debug, Component)]
#[storage(VecStorage)]
struct Name(String);

struct Move;

impl<'a> System<'a> for Move {
    type SystemData = (
        WriteExpect<'a, MoveScripts>,
        WriteStorage<'a, Pos>,
        ReadStorage<'a, Vel>,
    );

    fn run(&mut self, (mut ms, mut pos, vel): Self::SystemData) {
        for move_script in &mut ms.0 {
            let mut guard = move_script.lock().unwrap();
            let ms_inner = guard.borrow_mut();
            for (pos, vel) in (&mut pos, &vel).join() {
                *pos = ms_inner.update_position((pos.0, pos.1, vel.0, vel.1));
            }
        }
    }
}

struct ShoutOut;

impl<'a> System<'a> for ShoutOut {
    type SystemData = (ReadStorage<'a, Name>, ReadStorage<'a, Pos>);

    fn run(&mut self, (name, pos): Self::SystemData) {
        for (name, pos) in (&name, &pos).join() {
            println!("{} is at {}, {}!", name.0, pos.0, pos.1);
        }
    }
}

static MOVE_SCRIPT_1: &'static [u8] = include_bytes!(
    "../../specs-scripts/specs-script/target/wasm32-unknown-unknown/release/specs-script.wasm"
);
static MOVE_SCRIPT_2: &'static [u8] = include_bytes!(
    "../../specs-scripts/move-script-2/target/wasm32-unknown-unknown/release/move-script-2.wasm"
);

struct ScriptingSystem {
    instance: Instance,
}

struct MoveScript {
    system: ScriptingSystem,
}

impl MoveScript {
    pub fn update_position(&mut self, (arg1, arg2, arg3, arg4): (f32, f32, f32, f32)) -> Pos {
        let update_position = self
            .system
            .instance
            .func::<(f32, f32, f32, f32), u64>("update_position")
            .unwrap();
        let result = update_position.call(arg1, arg2, arg3, arg4).unwrap();

        unsafe { std::mem::transmute(result) }
    }
}

struct MoveScripts(Vec<Mutex<MoveScript>>);

impl MoveScripts {
    pub fn new(move_scripts: Vec<ScriptingSystem>) -> Self {
        let mut inner = vec![];
        for script in move_scripts.into_iter() {
            let ms = MoveScript { system: script };
            inner.push(Mutex::new(ms));
        }
        Self(inner)
    }
}

fn main() {
    let import_object = imports! {};

    let move_instance_1 = instantiate(MOVE_SCRIPT_1, &import_object).unwrap();
    let move_instance_2 = instantiate(MOVE_SCRIPT_2, &import_object).unwrap();
    let move_scripts = MoveScripts::new(vec![
        ScriptingSystem {
            instance: move_instance_1,
        },
        ScriptingSystem {
            instance: move_instance_2,
        },
    ]);

    let mut world = World::new();
    world.add_resource(move_scripts);

    let mut dispatcher = DispatcherBuilder::new()
        .with(Move, "move", &[])
        .with(ShoutOut, "shout_out", &["move"])
        .build();

    dispatcher.setup(&mut world);

    world
        .create_entity()
        .with(Vel(2.0, 0.))
        .with(Pos(0.0, 0.))
        .with(Name("Ario".to_string()))
        .build();
    world
        .create_entity()
        .with(Vel(4.0, 1.))
        .with(Pos(1.6, 1.))
        .with(Name("Bario".to_string()))
        .build();
    world
        .create_entity()
        .with(Vel(1.5, 2.))
        .with(Pos(5.4, -2.))
        .with(Name("Cario".to_string()))
        .build();

    world.create_entity().with(Pos(2., 2.)).build();

    for _ in 0..30 {
        dispatcher.dispatch_par(&world);
        world.maintain();
    }
}

Attached is the updated specs example posted in amethyst/amethyst#1729 ; but this one only relies on Instance and Func implementing Send

[MarkMcCaskey]

Let me expand a bit on this, the reason why we currently think Instance should be Send but not Sync is that Instance contains a *mut Ctx and with &Instance we can get Func<'a, Args, Rets, Wasm> which also contains a *mut Ctx. If we make getting a Func require a &mut Instance then the pattern of caching wasm functions as variables is not possible because of borrowing.

It's maybe possible to fix this by requiring &mut Instance and doing this function generation in bulk... We could hide that behind a macro or something. Though I suspect this will have a few other problems. Even if it does work, the API is less flexible and less pleasant to use. This is something we need to look into more.

If we can make all methods on Instance that pass around the *mut Ctx take a &mut Instance rather than a &Instance than we can Instance Send and Sync because you need &mut which requires exclusive access. This is functionally equivalent to what we currently have, it just may be a bit more ergonomic.

One last note, the example above is almost certainly slower. The acquisition of an uncontested mutex shouldn't take long, especially because it's just once per Wasm Instance per System that uses Wasm. But putting it in a mutex made the ownership more complex and we couldn't store the Func<'a, (f32, f32, f32, f32), u64> in the struct directly anymore so we have to look it up every time, which to my knowledge adds overhead.

[caelunshun]

Yes, I agree that there is no real need to implement Sync in addition to Send, especially giving the presumed difficulty of doing so. The only drawback this would have is not being able to call the same module concurrently, but that sacrifice is worth the improved ergonomics, as far as I'm concerned.

Also, I would imagine the Func could still be cached by doing something like this:

struct WrappedInstance<'a> {
    instance: Instance,
    cached_function: Func<'a, (f32, f32, f32, f32), u64>,
}

// And then use access the instance through `Mutex<WrappedInstance<'a>>`

This might cause borrow checker issues because of the lifetime parameter on 'WrappedInstance`, but I'm sure there's a way around that.

[MarkMcCaskey]

@caelunshun Yeah, ideally we'd like to be able to share the module (the compiled Wasm) but not anything that has a *mut Ctx and can execute directly and cause race conditions. This is something we're looking into more.

In the code above I had that but with one layer of indirection and wasn't able to get it work directly. It seems like something that rental would fix, but that's not great.

The issue is that the lifetime 'a is borrowing from Instance, so Rust gets unhappy because Instance is owned?

struct ScriptingSystem<'a> {
    instance: Instance,
    _tag: PhantomData<'a>,
}

struct MoveScript<'a> {
    system: ScriptingSystem<'a>,
    cached_function: Func<'a, (f32, f32, f32, f32), u64>,
}

Was roughly what I ended up with before and I wasn't able to make it work; maybe I'm missing something obvious though.

Edit: I think this is a really important ergonomic point, so I'll look into it more today and come up with a better justification for why I couldn't get it to work

[MarkMcCaskey]

Instance is now Send on master! We're still looking for feedback on this and we'd love to hear about any issues anyone has trying to use this!

[mikevoronov]

Really cool work! In Fluence we are trying to use Wasmer in such that we need state of Wasmer to be preserved globally. And thread local isn't suitable because it is difficult to guarantee lack of context switching on our nodes.

So it would be great to make new minor release of Wasmer with this feature :).