From a1047eeb95e80ef4d04b2b1a2abe767b95262b02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 12 Oct 2023 00:09:04 +0200 Subject: [PATCH 1/5] sc-executor: Increase maximum instance count It was reported that the maximum instance count of `32` isn't enough. By default `wasmtime` uses an instance count of `1000`. So, it should be safe to increase the instance count to `1000`. --- substrate/client/executor/wasmtime/src/runtime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/executor/wasmtime/src/runtime.rs b/substrate/client/executor/wasmtime/src/runtime.rs index ae78137959be..77d2932446d7 100644 --- a/substrate/client/executor/wasmtime/src/runtime.rs +++ b/substrate/client/executor/wasmtime/src/runtime.rs @@ -277,7 +277,7 @@ fn common_config(semantics: &Semantics) -> std::result::Result Date: Thu, 12 Oct 2023 00:26:01 +0200 Subject: [PATCH 2/5] Allow only 32 in the CLI and decrease the maximum number of instances to 64 --- substrate/client/cli/src/params/runtime_params.rs | 6 +++--- substrate/client/executor/wasmtime/src/runtime.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/client/cli/src/params/runtime_params.rs b/substrate/client/cli/src/params/runtime_params.rs index 79035fc7d4c5..07009a96ee6e 100644 --- a/substrate/client/cli/src/params/runtime_params.rs +++ b/substrate/client/cli/src/params/runtime_params.rs @@ -22,7 +22,7 @@ use std::str::FromStr; /// Parameters used to config runtime. #[derive(Debug, Clone, Args)] pub struct RuntimeParams { - /// The size of the instances cache for each runtime. The values higher than 256 are illegal. + /// The size of the instances cache for each runtime. The values higher than 32 are illegal. #[arg(long, default_value_t = 8, value_parser = parse_max_runtime_instances)] pub max_runtime_instances: usize, @@ -35,8 +35,8 @@ fn parse_max_runtime_instances(s: &str) -> Result { let max_runtime_instances = usize::from_str(s) .map_err(|_err| format!("Illegal `--max-runtime-instances` value: {s}"))?; - if max_runtime_instances > 256 { - Err(format!("Illegal `--max-runtime-instances` value: {max_runtime_instances} is more than the allowed maximum of `256` ")) + if max_runtime_instances > 32 { + Err(format!("Illegal `--max-runtime-instances` value: {max_runtime_instances} is more than the allowed maximum of `32` ")) } else { Ok(max_runtime_instances) } diff --git a/substrate/client/executor/wasmtime/src/runtime.rs b/substrate/client/executor/wasmtime/src/runtime.rs index 77d2932446d7..b9978f822681 100644 --- a/substrate/client/executor/wasmtime/src/runtime.rs +++ b/substrate/client/executor/wasmtime/src/runtime.rs @@ -277,7 +277,7 @@ fn common_config(semantics: &Semantics) -> std::result::Result Date: Sun, 15 Oct 2023 01:13:00 +0200 Subject: [PATCH 3/5] Ensure we respect the upper maximum number of instances --- Cargo.lock | 1 + substrate/client/executor/wasmtime/Cargo.toml | 1 + .../executor/wasmtime/src/instance_wrapper.rs | 14 +++- .../client/executor/wasmtime/src/runtime.rs | 66 ++++++++++++++++++- 4 files changed, 76 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 58bacc9db739..e6d00085b89a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15054,6 +15054,7 @@ dependencies = [ "libc", "log", "parity-scale-codec", + "parking_lot 0.12.1", "paste", "rustix 0.36.15", "sc-allocator", diff --git a/substrate/client/executor/wasmtime/Cargo.toml b/substrate/client/executor/wasmtime/Cargo.toml index fee1afc96666..261d52c0ede3 100644 --- a/substrate/client/executor/wasmtime/Cargo.toml +++ b/substrate/client/executor/wasmtime/Cargo.toml @@ -16,6 +16,7 @@ targets = ["x86_64-unknown-linux-gnu"] log = "0.4.17" cfg-if = "1.0" libc = "0.2.121" +parking_lot = "0.12.1" # When bumping wasmtime do not forget to also bump rustix # to exactly the same version as used by wasmtime! diff --git a/substrate/client/executor/wasmtime/src/instance_wrapper.rs b/substrate/client/executor/wasmtime/src/instance_wrapper.rs index acc799061c27..daff8ab22761 100644 --- a/substrate/client/executor/wasmtime/src/instance_wrapper.rs +++ b/substrate/client/executor/wasmtime/src/instance_wrapper.rs @@ -19,7 +19,9 @@ //! Defines data and logic needed for interaction with an WebAssembly instance of a substrate //! runtime module. -use crate::runtime::{Store, StoreData}; +use std::sync::Arc; + +use crate::runtime::{InstanceCounter, ReleaseInstanceHandle, Store, StoreData}; use sc_executor_common::{ error::{Backtrace, Error, MessageWithBacktrace, Result, WasmError}, wasm_runtime::InvokeMethod, @@ -154,11 +156,17 @@ impl sc_allocator::Memory for MemoryWrapper<'_, C> { pub struct InstanceWrapper { instance: Instance, store: Store, + _release_instance_handle: ReleaseInstanceHandle, } impl InstanceWrapper { - pub(crate) fn new(engine: &Engine, instance_pre: &InstancePre) -> Result { + pub(crate) fn new( + engine: &Engine, + instance_pre: &InstancePre, + instance_counter: Arc, + ) -> Result { let mut store = Store::new(engine, Default::default()); + let _release_instance_handle = instance_counter.acquire_instance(); let instance = instance_pre.instantiate(&mut store).map_err(|error| { WasmError::Other(format!( "failed to instantiate a new WASM module instance: {:#}", @@ -172,7 +180,7 @@ impl InstanceWrapper { store.data_mut().memory = Some(memory); store.data_mut().table = table; - Ok(InstanceWrapper { instance, store }) + Ok(InstanceWrapper { instance, store, _release_instance_handle }) } /// Resolves a substrate entrypoint by the given name. diff --git a/substrate/client/executor/wasmtime/src/runtime.rs b/substrate/client/executor/wasmtime/src/runtime.rs index b9978f822681..466ecc29c5ed 100644 --- a/substrate/client/executor/wasmtime/src/runtime.rs +++ b/substrate/client/executor/wasmtime/src/runtime.rs @@ -24,6 +24,7 @@ use crate::{ util::{self, replace_strategy_if_broken}, }; +use parking_lot::Mutex; use sc_allocator::{AllocationStats, FreeingBumpHeapAllocator}; use sc_executor_common::{ error::{Error, Result, WasmError}, @@ -42,6 +43,8 @@ use std::{ }; use wasmtime::{AsContext, Engine, Memory, Table}; +const MAX_INSTANCE_COUNT: u32 = 64; + #[derive(Default)] pub(crate) struct StoreData { /// This will only be set when we call into the runtime. @@ -73,11 +76,61 @@ enum Strategy { struct InstanceCreator { engine: Engine, instance_pre: Arc>, + instance_counter: Arc, } impl InstanceCreator { fn instantiate(&mut self) -> Result { - InstanceWrapper::new(&self.engine, &self.instance_pre) + InstanceWrapper::new(&self.engine, &self.instance_pre, self.instance_counter.clone()) + } +} + +/// A handle for releasing an instance acquired by [`InstanceCounter::acquire_instance`]. +pub(crate) struct ReleaseInstanceHandle { + counter: Arc, +} + +impl Drop for ReleaseInstanceHandle { + fn drop(&mut self) { + { + let mut counter = self.counter.counter.lock(); + *counter = counter.saturating_sub(1); + } + + self.counter.wait_for_instance.notify_one(); + } +} + +/// Keeps track on the number of parallel instances. +/// +/// The runtime cache keeps track on the number of parallel instances. The maximum number in the +/// cache is less than what we have configured as [`MAX_INSTANCE_COUNT`] for wasmtime. However, the +/// cache will create on demand instances if required. This instance counter will ensure that we are +/// blocking when we are trying to create too many instances. +#[derive(Default)] +pub(crate) struct InstanceCounter { + counter: Mutex, + wait_for_instance: parking_lot::Condvar, +} + +impl InstanceCounter { + /// Acquire an instance. + /// + /// Blocks if there is no free instance available. + /// + /// The returned [`ReleaseInstanceHandle`] should be dropped when the instance isn't used + /// anymore. + pub fn acquire_instance(self: Arc) -> ReleaseInstanceHandle { + let mut counter = self.counter.lock(); + + if *counter + 1 < MAX_INSTANCE_COUNT { + *counter += 1; + } else { + self.wait_for_instance.wait(&mut counter); + *counter += 1; + } + + ReleaseInstanceHandle { counter: self.clone() } } } @@ -87,6 +140,7 @@ pub struct WasmtimeRuntime { engine: Engine, instance_pre: Arc>, instantiation_strategy: InternalInstantiationStrategy, + instance_counter: Arc, } impl WasmModule for WasmtimeRuntime { @@ -95,6 +149,7 @@ impl WasmModule for WasmtimeRuntime { InternalInstantiationStrategy::Builtin => Strategy::RecreateInstance(InstanceCreator { engine: self.engine.clone(), instance_pre: self.instance_pre.clone(), + instance_counter: self.instance_counter.clone(), }), }; @@ -277,7 +332,7 @@ fn common_config(semantics: &Semantics) -> std::result::Result Date: Sun, 22 Oct 2023 00:42:57 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Koute --- substrate/client/executor/wasmtime/src/instance_wrapper.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/substrate/client/executor/wasmtime/src/instance_wrapper.rs b/substrate/client/executor/wasmtime/src/instance_wrapper.rs index daff8ab22761..8852532adbca 100644 --- a/substrate/client/executor/wasmtime/src/instance_wrapper.rs +++ b/substrate/client/executor/wasmtime/src/instance_wrapper.rs @@ -156,6 +156,9 @@ impl sc_allocator::Memory for MemoryWrapper<'_, C> { pub struct InstanceWrapper { instance: Instance, store: Store, + // NOTE: We want to decrement the instance counter *after* the store has been dropped + // to avoid a potential race condition, so this field must always be kept + // as the last field in the struct! _release_instance_handle: ReleaseInstanceHandle, } @@ -165,8 +168,8 @@ impl InstanceWrapper { instance_pre: &InstancePre, instance_counter: Arc, ) -> Result { - let mut store = Store::new(engine, Default::default()); let _release_instance_handle = instance_counter.acquire_instance(); + let mut store = Store::new(engine, Default::default()); let instance = instance_pre.instantiate(&mut store).map_err(|error| { WasmError::Other(format!( "failed to instantiate a new WASM module instance: {:#}", From 3aaee71f9a6f65222d4f7f298af6c1fce78bc05d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sun, 22 Oct 2023 00:45:33 +0200 Subject: [PATCH 5/5] Update substrate/client/executor/wasmtime/src/runtime.rs Co-authored-by: Koute --- substrate/client/executor/wasmtime/src/runtime.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/substrate/client/executor/wasmtime/src/runtime.rs b/substrate/client/executor/wasmtime/src/runtime.rs index 466ecc29c5ed..ac88663f4e79 100644 --- a/substrate/client/executor/wasmtime/src/runtime.rs +++ b/substrate/client/executor/wasmtime/src/runtime.rs @@ -123,12 +123,10 @@ impl InstanceCounter { pub fn acquire_instance(self: Arc) -> ReleaseInstanceHandle { let mut counter = self.counter.lock(); - if *counter + 1 < MAX_INSTANCE_COUNT { - *counter += 1; - } else { + while *counter >= MAX_INSTANCE_COUNT { self.wait_for_instance.wait(&mut counter); - *counter += 1; } + *counter += 1; ReleaseInstanceHandle { counter: self.clone() } }