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

make ResourceLimiter operate on Store data; add hooks for entering and exiting native code #2952

Merged
merged 11 commits into from
Jun 8, 2021

Conversation

pchickey
Copy link
Collaborator

Based on #2897 - draft until that PR lands.

This PR does two things:

  • The ResourceLimiter trait is now implemented on the data inside a Store rather than as a standalone owned type. This change required changing the structure of the Store internals a bunch. Additionally, the trait is now defined in wasmtime_runtime and re-exported by wasmtime.
  • Two new functions, entering_native_code_hook and exiting_native_code_hook, are added to Store. These functions get to operate on the data inside a Store much like resource limiter's mut methods. These functions give embedding the opportunity to measure time (or other resources) and trap before entering any native code (i.e. a Func) linked into the Store, and again before returning from that native code.

@pchickey pchickey marked this pull request as draft May 29, 2021 00:00
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator wasmtime:c-api Issues pertaining to the C API. labels May 29, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

In preparation of changing wasmtime::ResourceLimiter to be a re-export
of this definition, because translating between two traits was causing
problems elsewhere.
@pchickey pchickey marked this pull request as ready for review June 3, 2021 22:01
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Ok reading again now that the diff is a bit smaller, this is still looking good. (modulo the Innermost which I don't know how to make better and isn't really all that bad).

Can you add some tests thouggh for the enter/exit hooks? Ideally we'd have tests ensuring that they fire for all kinds of created host functions (e.g. Linker-defined, Func-defined, etc).

Finally, do you think it would make sense to run the hooks around the initial entry point into wasm as well? (e.g. the single entry point into wasm we have in func.rs).

@github-actions github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself labels Jun 3, 2021
@github-actions
Copy link

github-actions bot commented Jun 3, 2021

Subscribe to Label Action

cc @fitzgen, @peterhuene

This issue or pull request has been labeled: "fuzzing", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: fuzzing
  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

let mut store = Store::new(&engine, ());
store.limiter(StoreLimitsBuilder::new().memories(10).build());
let mut store = Store::new(&engine, StoreLimitsBuilder::new().memories(10).build());
store.limiter(|s| s as &mut dyn ResourceLimiter);
Copy link
Member

Choose a reason for hiding this comment

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

Is the as &mut dyn ResourceLimiter required here in that rustc can't infer it?

Copy link
Contributor

Choose a reason for hiding this comment

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

|s| s doesn't attempt to coerce s AFAIK.

@alexcrichton
Copy link
Member

Locally I've got two benchmarks for changes like this:

host -> wasm overhead
fn host_to_wasm_overhead() -> anyhow::Result<()> {
    use wasmtime::*;
    let engine = Engine::default();
    let module = Module::new(
        &engine,
        r#"
            (module
                (func (export "run") (param i32 i32) (result i32)
                    local.get 0
                    local.get 1
                    i32.add
                )
            )
        "#,
    )?;
    let mut store = Store::new(&engine, ());
    let instance = Instance::new(&mut store, &module, &[])?;
    let run = instance.get_typed_func::<(i32, i32), i32, _>(&mut store, "run")?;
    let now = std::time::Instant::now();
    let total = 100_000_000;
    for _ in 0..total {
        run.call(&mut store, (1, 1))?;
    }
    let time = now.elapsed();
    println!(
        "use Time: {:?}, each:{} ns/op",
        time,
        time.as_nanos() / (total as u128)
    );
    Ok(())
}
wasm -> host overhead
fn wasm_to_host() -> anyhow::Result<()> {
    use wasmtime::*;
    let engine = Engine::default();
    let module = Module::new(
        &engine,
        r#"
            (module
                (import "" "add" (func $add (param i32 i32) (result i32)))

                (func (export "run") (param i32)
                    loop
                        i32.const 1
                        i32.const 2
                        call 0
                        drop

                        local.get 0
                        i32.const -1
                        i32.add
                        local.tee 0
                        br_if 0
                    end
                ))
        "#,
    )?;
    let mut store = Store::new(&engine, ());
    let func = Func::wrap(&mut store, |a: u32, b: u32| a + b);
    let instance = Instance::new(&mut store, &module, &[func.into()])?;
    let run = instance.get_typed_func::<i32, (), _>(&mut store, "run")?;
    let now = std::time::Instant::now();
    let total = 1_000_000_000;
    run.call(&mut store, total)?;
    let time = now.elapsed();
    println!(
        "use Time: {:?}, each:{} ns/op",
        time,
        time.as_nanos() / (total as u128)
    );
    Ok(())
}

The numbers I got for this PR on my local macOS machine are:

before after
host -> wasm 29ns 30ns
wasm -> host 1ns 2ns

So it looks like adding these unconditional branches has pretty negligible overhead. If it really matters in some use case we could always add a compile-time option to disable them, but otherwise I don't think there's too much impact.

One thing that's required, though, is to add #[inline] to a few more methods. I noticed this when profiling and gathered the numbers with some local changes to this PR. The reason is that StoreInner methods which previously had a generic in scope are now concrete on StoreInnermost so they require #[inline] to get cross-crate inlined. The ones I added inline too are:

  • StoreInnermost::signal_handler
  • StoreInnermost::vminterrupts
  • StoreInnermost::async_cx
  • StoreInnermost::default_callee
  • StoreInnermost::async_support
  • StoreInnermost::engine
  • StoreInnermost::data
  • StoreInnermost::data_mut

@alexcrichton alexcrichton merged commit 8b4bdf9 into main Jun 8, 2021
@alexcrichton alexcrichton deleted the pch/limiter_in_store_data branch June 8, 2021 14:37
@alexcrichton
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants