From ed385054ce708e501a12d1087e6b60cfab8dd0b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sun, 22 Oct 2023 10:44:34 +0200 Subject: [PATCH] sc-executor: Increase maximum instance count (#1856) Changes the maximum instances count for `wasmtime` to `64`. It also allows to only pass in maximum `32` for `--max-runtime-instances` as `256` was way too big. With `64` instances in total and `32` that can be configured in maximum, there should be enough space to accommodate for extra instances that are may required to be allocated adhoc. --------- Co-authored-by: Koute --- .../client/cli/src/params/runtime_params.rs | 6 +- substrate/client/executor/wasmtime/Cargo.toml | 1 + .../executor/wasmtime/src/instance_wrapper.rs | 17 ++++- .../client/executor/wasmtime/src/runtime.rs | 64 ++++++++++++++++++- 4 files changed, 79 insertions(+), 9 deletions(-) diff --git a/substrate/client/cli/src/params/runtime_params.rs b/substrate/client/cli/src/params/runtime_params.rs index 79035fc7d4c5d..07009a96ee6e6 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/Cargo.toml b/substrate/client/executor/wasmtime/Cargo.toml index fee1afc96666b..261d52c0ede32 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 acc799061c27c..8852532adbcaa 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,10 +156,19 @@ 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, } 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 _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!( @@ -172,7 +183,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 ae78137959be1..ac88663f4e791 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,59 @@ 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(); + + while *counter >= MAX_INSTANCE_COUNT { + self.wait_for_instance.wait(&mut counter); + } + *counter += 1; + + ReleaseInstanceHandle { counter: self.clone() } } } @@ -87,6 +138,7 @@ pub struct WasmtimeRuntime { engine: Engine, instance_pre: Arc>, instantiation_strategy: InternalInstantiationStrategy, + instance_counter: Arc, } impl WasmModule for WasmtimeRuntime { @@ -95,6 +147,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 +330,7 @@ fn common_config(semantics: &Semantics) -> std::result::Result