Statically register host WASM functions #10394

koute · 2021-11-30T12:54:22Z

skip check-dependent-cumulus

polkadot companion: paritytech/polkadot#4471

This PR changes how we register the host WASM functions exposed to the runtime. Currently we register them dynamically with wasmtime::Func::new, while this PR makes it so that we statically register them with wasmtime::Func::wrap.

Why? Let me quote wasmtime docs:

This function [Func::wrap] will create a new Func which, when called, will execute the given Rust closure. Unlike Func::new the target function being called is known statically so the type signature can be inferred. [...] Note that when using this API, the intention is to create as thin of a layer as possible for when WebAssembly calls the function provided. With sufficient inlining and optimization the WebAssembly will call straight into func provided, with no extra fluff entailed.

And indeed, using Func::wrap is significantly more efficient than using Func::new. Granted, we probably don't make enough calls from the WASM runtime for this to make a significant difference when those functions are called, however where it makes a huge difference is when we're instantiating new WASM instances.

When using Func::new every time a host function is registered wasmtime has to JIT a new trampoline for it, and this is extremely slow; when using Func::wrap it doesn't, and from what I can see this cuts down the instantiation time by over 90%.

Of course this also doesn't make a huge difference in practice right now since we're using our own instance reuse mechanism, however this is absolutely necessary if we'd ever want to switch to wasmtime's native instance pooling. (Without this PR even with wasmtimes instance pooling enabled the cost of JITing those trampolines completely dominates the execution time.)

EDIT(pep): Closes #8959

gilescope · 2021-11-30T13:45:56Z

client/executor/src/native_executor.rs

+		let mut base = Base::host_functions();
+		let overlay = Overlay::host_functions();
+		base.retain(|host_fn| {
+			!overlay.iter().any(|ext_host_fn| host_fn.name() == ext_host_fn.name())


do we want to pull the overlay names into a set or do we think there will be very few of them?

That's how it was originally, so it's probably not a big deal? Nevertheless there aren't that many of them, and I haven't seen this pop up in any of profiles.

I can rework this to use a HashSet if you really want though.

primitives/wasm-interface/src/lib.rs

pepyakin · 2021-12-04T12:40:28Z

primitives/wasm-interface/src/lib.rs

+		type State;
+		type Error;
+		type FunctionContext: FunctionContext;
+		fn with_function_context<R>(caller: wasmtime::Caller<Self::State>, callback: impl FnOnce(&mut dyn FunctionContext) -> R) -> R;


Just out of curiosity, why dynamic object and not Self::FunctionContext?

Making it Self::FunctionContext significantly complicates the lifetimes everywhere, and from the top of my head I'm not even sure if it's actually possible to use a concrete type here without GATs (since Caller actually takes a lifetime which has to be passed to the Self::FunctionContext, it's just elided here). Also, rest of the traits (e.g. host::FromFFIValue) and the dynamic dispatch machinery (Function::execute) take it as &dyn FunctionContext anyway.

If we really want a concrete type here I'd suggest that we should ignore it for now and just revisit this once GATs are stabilized.

Nah, all is fine. Was just curious

primitives/runtime-interface/proc-macro/src/runtime_interface/host_function_interface.rs

client/executor/wasmtime/src/runtime.rs

pepyakin · 2021-12-04T13:27:17Z

FWIW, this will require polkadot companion

pepyakin

This looks good

Maybe this will appease cargo-unleash?

This reverts commit 0f7ccf8.

bkchr

Looks good, just some nitpicks here and there.

Ty for the work!

primitives/wasm-interface/src/lib.rs

bkchr · 2021-12-13T08:48:19Z

primitives/wasm-interface/src/lib.rs

+					fn_name: &str,
+					func: impl wasmtime::IntoFunc<Self::State, Params, Results> + 'static,
+				) -> core::result::Result<(), Self::Error> {
+					if self.seen.contains(fn_name) {


That is to prevent double registration?

Maybe some debug log would be nice that says that we filtered this one out.

Yes, but only in a sense that since this is explicitly used to override the functions from the Base with the functions from the Overlay it is expected that there will be some duplicates.

Yeah I guess I can add a debug log here.

Okay, I've also added a little bit more strict checking here to disambiguate between duplicates in Base vs duplicates in Overlay.

bkchr · 2021-12-13T08:54:11Z

primitives/runtime-interface/src/pass_by.rs

-	type FFIType = u8;
+	type FFIType = u32;


Just to be sure. This was internally passed before as u32 anyway, because wasm types don't know u8 "natively"? So, we don't change it?

Yes. WASM FFI doesn't support 8-bit and 16-bit integers natively, so it was being converted into an u32 later anyway. So I've put the extra WasmTy bound on the FFIType so that it will be guaranteed that's actually FFI-compatible and moved the conversion into the native WASM FFI type earlier.

primitives/runtime-interface/proc-macro/src/runtime_interface/host_function_interface.rs

bkchr · 2021-12-13T10:45:52Z

client/executor/wasmtime/src/imports.rs

+		&mut self,
+		fn_name: &str,
+		func: impl wasmtime::IntoFunc<Self::State, Params, Results>,
+	) -> Result<(), Self::Error> {


Why do you return a result here? To be future proof?

Hmm... good question! (:

Currently it can indeed only return an Ok; IIRC it's a vestigial remain from before one of the refactorings I did before I pushed this PR out where it could actually return an error.

primitives/runtime-interface/proc-macro/src/runtime_interface/host_function_interface.rs

koute · 2021-12-14T08:23:43Z

bot merge

paritytech-processbot · 2021-12-14T08:23:50Z

Error: Github API says paritytech/polkadot#4471 is not mergeable

koute · 2021-12-14T08:26:29Z

bot merge

* Statically register host WASM functions * Fix `substrate-test-client` compilation * Move `ExtendedHostFunctions` to `sp-wasm-interface` * Fix `sp-runtime-interface` tests' compilation * Fix `sc-executor-wasmtime` tests' compilation * Use `runtime_interface` macro in `test-runner` * Fix `sc-executor` tests' compilation * Reformatting/`rustfmt` * Add an extra comment regarding the `H` generic arg in `create_runtime` * Even more `rustfmt` * Depend on `wasmtime` without default features in `sp-wasm-interface` * Bump version of `sp-wasm-interface` to 4.0.1 * Bump `sp-wasm-interface` in `Cargo.lock` too * Bump all of the `sp-wasm-interface` requirements to 4.0.1 Maybe this will appease cargo-unleash? * Revert "Bump all of the `sp-wasm-interface` requirements to 4.0.1" This reverts commit 0f7ccf8. * Make `cargo-unleash` happy (maybe) * Use `cargo-unleash` to bump the crates' versions * Align to review comments

Statically register host WASM functions

a6b9624

koute requested review from pepyakin and bkchr November 30, 2021 12:54

Fix substrate-test-client compilation

1d8649f

gilescope reviewed Nov 30, 2021

View reviewed changes

koute mentioned this pull request Dec 2, 2021

Investigate instance pooling strategy in wasmtime #10244

Closed

koute added 4 commits December 2, 2021 20:18

Move ExtendedHostFunctions to sp-wasm-interface

2a4658f

Fix sp-runtime-interface tests' compilation

878d512

Fix sc-executor-wasmtime tests' compilation

7efc4f5

Use runtime_interface macro in test-runner

570788d

pepyakin reviewed Dec 4, 2021

View reviewed changes

koute added 3 commits December 6, 2021 19:05

Fix sc-executor tests' compilation

7610cde

Reformatting/rustfmt

0c51a64

Add an extra comment regarding the H generic arg in create_runtime

963f9e7

koute mentioned this pull request Dec 6, 2021

Companion for Substrate#10394 paritytech/polkadot#4471

Merged

koute added 3 commits December 6, 2021 19:40

Even more rustfmt

76e8c96

Depend on wasmtime without default features in sp-wasm-interface

e853225

Merge branch 'master' into master_statically_register_wasmtime_ffi

df38788

pepyakin approved these changes Dec 6, 2021

View reviewed changes

koute added 4 commits December 9, 2021 19:50

Merge branch 'master' into master_statically_register_wasmtime_ffi

3699f6d

Merge branch 'master' into master_statically_register_wasmtime_ffi

cd14ed1

Bump version of sp-wasm-interface to 4.0.1

f825113

Bump sp-wasm-interface in Cargo.lock too

8ffb42d

koute added 5 commits December 10, 2021 20:28

Bump all of the sp-wasm-interface requirements to 4.0.1

0f7ccf8

Maybe this will appease cargo-unleash?

Revert "Bump all of the sp-wasm-interface requirements to 4.0.1"

e429bb9

This reverts commit 0f7ccf8.

Make cargo-unleash happy (maybe)

9ce2fb7

Use cargo-unleash to bump the crates' versions

f9019cd

Merge branch 'master' into master_statically_register_wasmtime_ffi

acc2835

bkchr approved these changes Dec 13, 2021

View reviewed changes

koute added 2 commits December 13, 2021 23:38

Align to review comments

61ce0b2

Merge branch 'master' into master_statically_register_wasmtime_ffi

bdd38da

paritytech-processbot bot merged commit 49a4b18 into paritytech:master Dec 14, 2021

librelois mentioned this pull request Feb 7, 2022

Update substrate/polkadot/cumulus from v0.9.15 to v0.9.16 moonbeam-foundation/moonbeam#1259

Closed

koute mentioned this pull request Sep 29, 2022

Member Request polkadot-fellows/seeding#27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Statically register host WASM functions #10394

Statically register host WASM functions #10394

koute commented Nov 30, 2021 •

edited by bkchr

Loading

gilescope Nov 30, 2021

koute Dec 2, 2021

pepyakin Dec 4, 2021

koute Dec 6, 2021

pepyakin Dec 6, 2021

pepyakin commented Dec 4, 2021

pepyakin left a comment

bkchr left a comment

bkchr Dec 13, 2021

koute Dec 13, 2021

koute Dec 13, 2021

bkchr Dec 13, 2021

koute Dec 13, 2021

bkchr Dec 13, 2021

koute Dec 13, 2021

koute commented Dec 14, 2021

paritytech-processbot bot commented Dec 14, 2021

koute commented Dec 14, 2021

Statically register host WASM functions #10394

Statically register host WASM functions #10394

Conversation

koute commented Nov 30, 2021 • edited by bkchr Loading

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

pepyakin commented Dec 4, 2021

pepyakin left a comment

Choose a reason for hiding this comment

bkchr 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

koute commented Dec 14, 2021

paritytech-processbot bot commented Dec 14, 2021

koute commented Dec 14, 2021

koute commented Nov 30, 2021 •

edited by bkchr

Loading