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

sc-executor: Increase maximum instance count #1856

Merged
merged 6 commits into from
Oct 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions substrate/client/cli/src/params/runtime_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Nit. what about a public const for 32?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was thinking about this as well. But then we would need to put this to some random place again or import another crate. So, I decided against this for now.

#[arg(long, default_value_t = 8, value_parser = parse_max_runtime_instances)]
pub max_runtime_instances: usize,

Expand All @@ -35,8 +35,8 @@ fn parse_max_runtime_instances(s: &str) -> Result<usize, String> {
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)
}
Expand Down
1 change: 1 addition & 0 deletions substrate/client/executor/wasmtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
17 changes: 14 additions & 3 deletions substrate/client/executor/wasmtime/src/instance_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -154,10 +156,19 @@ impl<C: AsContextMut> 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,
bkchr marked this conversation as resolved.
Show resolved Hide resolved
}

impl InstanceWrapper {
pub(crate) fn new(engine: &Engine, instance_pre: &InstancePre<StoreData>) -> Result<Self> {
pub(crate) fn new(
engine: &Engine,
instance_pre: &InstancePre<StoreData>,
instance_counter: Arc<InstanceCounter>,
) -> Result<Self> {
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!(
Expand All @@ -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.
Expand Down
64 changes: 61 additions & 3 deletions substrate/client/executor/wasmtime/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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.
Expand Down Expand Up @@ -73,11 +76,59 @@ enum Strategy {
struct InstanceCreator {
engine: Engine,
instance_pre: Arc<wasmtime::InstancePre<StoreData>>,
instance_counter: Arc<InstanceCounter>,
}

impl InstanceCreator {
fn instantiate(&mut self) -> Result<InstanceWrapper> {
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<InstanceCounter>,
}

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<u32>,
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<Self>) -> 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() }
}
}

Expand All @@ -87,6 +138,7 @@ pub struct WasmtimeRuntime {
engine: Engine,
instance_pre: Arc<wasmtime::InstancePre<StoreData>>,
instantiation_strategy: InternalInstantiationStrategy,
instance_counter: Arc<InstanceCounter>,
}

impl WasmModule for WasmtimeRuntime {
Expand All @@ -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(),
}),
};

Expand Down Expand Up @@ -277,7 +330,7 @@ fn common_config(semantics: &Semantics) -> std::result::Result<wasmtime::Config,
.instance_memories(1)
// This determines how many instances of the module can be
// instantiated in parallel from the same `Module`.
.instance_count(32);
.instance_count(MAX_INSTANCE_COUNT);

config.allocation_strategy(wasmtime::InstanceAllocationStrategy::Pooling(pooling_config));
}
Expand Down Expand Up @@ -587,7 +640,12 @@ where
.instantiate_pre(&module)
.map_err(|e| WasmError::Other(format!("cannot preinstantiate module: {:#}", e)))?;

Ok(WasmtimeRuntime { engine, instance_pre: Arc::new(instance_pre), instantiation_strategy })
Ok(WasmtimeRuntime {
engine,
instance_pre: Arc::new(instance_pre),
instantiation_strategy,
instance_counter: Default::default(),
})
}

fn prepare_blob_for_compilation(
Expand Down