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

Introduced EngineRef and AsEngineRef trait #3378

Merged
merged 11 commits into from
Nov 30, 2022
Merged

Introduced EngineRef and AsEngineRef trait #3378

merged 11 commits into from
Nov 30, 2022

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Nov 25, 2022

Description

Introduced EngineRef structure that is used to build a Module (instead of StoreRef) and the AsEngineRef trait.
Store implement this trait, so there will be no change in writen code even if it's an API change.

This allow to create a Module independantly of a Store, as it can be usefull for multi-thread scenario.

For #3372

@ptitSeb ptitSeb requested a review from syrusakbary as a code owner November 25, 2022 16:03
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Can we make the Engine also implement EngineRef? (that way will be a bit easier to use)

It seems we might also need to move/store tunables one level up to the Engine (instead of the store)

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Nov 25, 2022

Can we make the Engine also implement EngineRef? (that way will be a bit easier to use)

It seems we might also need to move/store tunables one level up to the Engine (instead of the store)

No, because Tunables are not linked to the Engine.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Nov 25, 2022

For reference, using https://github.com/dsherret/slow-wasm-example/tree/slow_drop I modified the main.rs to this:

extern crate wasmer;

use wasmer::{imports, BaseTunables, Cranelift, EngineBuilder, EngineRef, Instance, Module, Store};

// run build.sh in root directory to create this
static WASM: &'static [u8] =
    include_bytes!("../../formatter/target/wasm32-unknown-unknown/release/formatter.wasm");

fn main() {
    println!("Starting...");
    let start = {
        let mut threads = Vec::new();
        let start = std::time::Instant::now();
        let compiler = Cranelift::default();
        let engine = EngineBuilder::new(compiler).engine();
        let tunables = BaseTunables::for_target(&engine.target());
        let engineref = EngineRef::new(&engine, &tunables);
        let module = Module::new(&engineref, &WASM).unwrap();
        for _ in 0..10 {
            let module = module.clone();
            threads.push(std::thread::spawn(move || {
                // I haven't figure out how to create a module once and share it amongst the threads in wasmer 3
                let import_object = imports! {};
                let mut store = Store::default();
                let instance = Instance::new(&mut store, &module, &import_object).unwrap();
                let format = instance.exports.get_function("format").unwrap();
                let result = format.call(&mut store, &[]).unwrap();
                (store, instance, result)
            }));
        }
        let mut data = Vec::new();
        for thread in threads {
            data.push(thread.join().unwrap());
        }
        println!("Finished in {}ms...", start.elapsed().as_millis());
        eprintln!("Dropping...");
        std::time::Instant::now()
    };
    println!("Finished dropping in {}ms...", start.elapsed().as_millis());
}

I get performances similar to the wasmer 2.3 code

@syrusakbary
Copy link
Member

syrusakbary commented Nov 25, 2022

No, because Tunables are not linked to the Engine.

That's what I meant, can we link/move Tunables to the Engine?

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Nov 25, 2022

No, because Tunables are not linked to the Engine.

That's what I meant, can we link/move Tunables to the Engine?

I don't see how to do that without API Breaking changes.

@ptitSeb ptitSeb requested a review from syrusakbary November 30, 2022 10:40
@ptitSeb ptitSeb added this to the v3.1 milestone Nov 30, 2022
@ptitSeb
Copy link
Contributor Author

ptitSeb commented Nov 30, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 30, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants