Use CLI to configure max instances cache (#5177)

* Use CLI to configure max instances cache

* Fix tests

* Move default value into CLI

* Use SmallVec

* Apply review comments

* Get rid of `SmallVec`

Co-authored-by: Bastian Köcher <git@kchr.de>

Author: stanislav-tkach

bin/node/executor/benches/bench.rs

@@ -170,7 +170,8 @@ fn bench_execute_block(c: &mut Criterion) {
 				ExecutionMethod::Native => (true, WasmExecutionMethod::Interpreted),
 				ExecutionMethod::Wasm(wasm_method) => (false, *wasm_method),
 			};
-			let executor = NativeExecutor::new(wasm_method, None);
+
+			let executor = NativeExecutor::new(wasm_method, None, 8);
 			let runtime_code = RuntimeCode {
 				code_fetcher: &sp_core::traits::WrappedRuntimeCode(COMPACT_CODE.into()),
 				hash: vec![1, 2, 3],

bin/node/executor/tests/common.rs

@@ -58,7 +58,7 @@ pub fn from_block_number(n: u32) -> Header {
 }
 
 pub fn executor() -> NativeExecutor<Executor> {
-	NativeExecutor::new(WasmExecutionMethod::Interpreted, None)
+	NativeExecutor::new(WasmExecutionMethod::Interpreted, None, 8)
 }
 
 pub fn executor_call<

bin/node/runtime/src/lib.rs

@@ -83,7 +83,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
 	spec_version: 235,
-	impl_version: 0,
+	impl_version: 1,
 	apis: RUNTIME_API_VERSIONS,
 };
 

bin/node/testing/src/bench.rs

@@ -150,7 +150,7 @@ impl BenchDb {
 
 		let (client, backend) = sc_client_db::new_client(
 			db_config,
-			NativeExecutor::new(WasmExecutionMethod::Compiled, None),
+			NativeExecutor::new(WasmExecutionMethod::Compiled, None, 8),
 			&keyring.generate_genesis(),
 			None,
 			None,

client/cli/src/commands/runcmd.rs

@@ -265,7 +265,13 @@ pub struct RunCmd {
 		parse(from_os_str),
 		conflicts_with_all = &[ "password-interactive", "password" ]
 	)]
-	pub password_filename: Option<PathBuf>
+	pub password_filename: Option<PathBuf>,
+
+	/// The size of the instances cache for each runtime.
+	///
+	/// The default value is 8 and the values higher than 256 are ignored.
+	#[structopt(long = "max-runtime-instances", default_value = "8")]
+	pub max_runtime_instances: usize,
 }
 
 impl RunCmd {
@@ -435,6 +441,8 @@ impl RunCmd {
 		// Imply forced authoring on --dev
 		config.force_authoring = self.shared_params.dev || self.force_authoring;
 
+		config.max_runtime_instances = self.max_runtime_instances.min(256);
+
 		Ok(())
 	}
 

client/executor/src/integration_tests/mod.rs

@@ -46,6 +46,7 @@ fn call_in_wasm<E: Externalities>(
 		Some(1024),
 		HostFunctions::host_functions(),
 		true,
+		8,
 	);
 	executor.call_in_wasm(
 		&WASM_BINARY[..],
@@ -511,6 +512,7 @@ fn should_trap_when_heap_exhausted(wasm_method: WasmExecutionMethod) {
 		Some(17),  // `17` is the initial number of pages compiled into the binary.
 		HostFunctions::host_functions(),
 		true,
+		8,
 	);
 	executor.call_in_wasm(
 		&WASM_BINARY[..],

client/executor/src/lib.rs

@@ -78,6 +78,7 @@ mod tests {
 			Some(8),
 			sp_io::SubstrateHostFunctions::host_functions(),
 			true,
+			8,
 		);
 		let res = executor.call_in_wasm(
 			&WASM_BINARY[..],

client/executor/src/native_executor.rs

@@ -85,6 +85,8 @@ pub struct WasmExecutor {
 	cache: Arc<RuntimeCache>,
 	/// Allow missing function imports.
 	allow_missing_func_imports: bool,
+	/// The size of the instances cache.
+	max_runtime_instances: usize,
 }
 
 impl WasmExecutor {
@@ -101,13 +103,15 @@ impl WasmExecutor {
 		default_heap_pages: Option<u64>,
 		host_functions: Vec<&'static dyn Function>,
 		allow_missing_func_imports: bool,
+		max_runtime_instances: usize,
 	) -> Self {
 		WasmExecutor {
 			method,
 			default_heap_pages: default_heap_pages.unwrap_or(DEFAULT_HEAP_PAGES),
 			host_functions: Arc::new(host_functions),
-			cache: Arc::new(RuntimeCache::new()),
+			cache: Arc::new(RuntimeCache::new(max_runtime_instances)),
 			allow_missing_func_imports,
+			max_runtime_instances,
 		}
 	}
 
@@ -223,7 +227,11 @@ impl<D: NativeExecutionDispatch> NativeExecutor<D> {
 	///
 	/// `default_heap_pages` - Number of 64KB pages to allocate for Wasm execution.
 	/// 	Defaults to `DEFAULT_HEAP_PAGES` if `None` is provided.
-	pub fn new(fallback_method: WasmExecutionMethod, default_heap_pages: Option<u64>) -> Self {
+	pub fn new(
+		fallback_method: WasmExecutionMethod,
+		default_heap_pages: Option<u64>,
+		max_runtime_instances: usize,
+	) -> Self {
 		let mut host_functions = sp_io::SubstrateHostFunctions::host_functions();
 
 		// Add the custom host functions provided by the user.
@@ -233,6 +241,7 @@ impl<D: NativeExecutionDispatch> NativeExecutor<D> {
 			default_heap_pages,
 			host_functions,
 			false,
+			max_runtime_instances,
 		);
 
 		NativeExecutor {
@@ -463,7 +472,11 @@ mod tests {
 
 	#[test]
 	fn native_executor_registers_custom_interface() {
-		let executor = NativeExecutor::<MyExecutor>::new(WasmExecutionMethod::Interpreted, None);
+		let executor = NativeExecutor::<MyExecutor>::new(
+			WasmExecutionMethod::Interpreted,
+			None,
+			8,
+		);
 		my_interface::HostFunctions::host_functions().iter().for_each(|function| {
 			assert_eq!(
 				executor.wasm.host_functions.iter().filter(|f| f == &function).count(),

client/executor/src/wasm_runtime.rs

@@ -21,7 +21,7 @@
 
 use std::sync::Arc;
 use crate::error::{Error, WasmError};
-use parking_lot::{Mutex, RwLock};
+use parking_lot::Mutex;
 use codec::Decode;
 use sp_core::traits::{Externalities, RuntimeCode, FetchRuntimeCode};
 use sp_version::RuntimeVersion;
@@ -53,11 +53,77 @@ struct VersionedRuntime {
 	/// Runtime version according to `Core_version` if any.
 	version: Option<RuntimeVersion>,
 	/// Cached instance pool.
-	instances: RwLock<[Option<Arc<Mutex<Box<dyn WasmInstance>>>>; MAX_INSTANCES]>,
+	instances: Vec<Mutex<Option<Box<dyn WasmInstance>>>>,
+}
+
+impl VersionedRuntime {
+	/// Run the given closure `f` with an instance of this runtime.
+	fn with_instance<'c, R, F>(
+		&self,
+		ext: &mut dyn Externalities,
+		f: F,
+	) -> Result<R, Error>
+		where F: FnOnce(
+			&dyn WasmInstance,
+			Option<&RuntimeVersion>,
+			&mut dyn Externalities)
+		-> Result<R, Error>,
+	{
+		// Find a free instance
+		let instance = self.instances
+			.iter()
+			.enumerate()
+			.find_map(|(index, i)| i.try_lock().map(|i| (index, i)));
+
+		match instance {
+			Some((index, mut locked)) => {
+				let (instance, new_inst) = locked.take()
+					.map(|r| Ok((r, false)))
+					.unwrap_or_else(|| self.module.new_instance().map(|i| (i, true)))?;
+
+				let result = f(&*instance, self.version.as_ref(), ext);
+				if let Err(e) = &result {
+					if new_inst {
+						log::warn!(
+							target: "wasm-runtime",
+							"Fresh runtime instance failed with {:?}",
+							e,
+						)
+					} else {
+						log::warn!(
+							target: "wasm-runtime",
+							"Evicting failed runtime instance: {:?}",
+							e,
+						);
+					}
+				} else {
+					*locked = Some(instance);
+
+					if new_inst {
+						log::debug!(
+							target: "wasm-runtime",
+							"Allocated WASM instance {}/{}",
+							index + 1,
+							self.instances.len(),
+						);
+					}
+				}
+
+				result
+			},
+			None => {
+				log::warn!(target: "wasm-runtime", "Ran out of free WASM instances");
+
+				// Allocate a new instance
+				let instance = self.module.new_instance()?;
+
+				f(&*instance, self.version.as_ref(), ext)
+			}
+		}
+	}
 }
 
 const MAX_RUNTIMES: usize = 2;
-const MAX_INSTANCES: usize = 8;
 
 /// Cache for the runtimes.
 ///
@@ -69,20 +135,22 @@ const MAX_INSTANCES: usize = 8;
 /// the memory reset to the initial memory. So, one runtime instance is reused for every fetch
 /// request.
 ///
-/// For now the cache grows indefinitely, but that should be fine for now since runtimes can only be
-/// upgraded rarely and there are no other ways to make the node to execute some other runtime.
+/// The size of cache is equal to `MAX_RUNTIMES`.
 pub struct RuntimeCache {
 	/// A cache of runtimes along with metadata.
 	///
 	/// Runtimes sorted by recent usage. The most recently used is at the front.
 	runtimes: Mutex<[Option<Arc<VersionedRuntime>>; MAX_RUNTIMES]>,
+	/// The size of the instances cache for each runtime.
+	max_runtime_instances: usize,
 }
 
 impl RuntimeCache {
 	/// Creates a new instance of a runtimes cache.
-	pub fn new() -> RuntimeCache {
+	pub fn new(max_runtime_instances: usize) -> RuntimeCache {
 		RuntimeCache {
 			runtimes: Default::default(),
+			max_runtime_instances,
 		}
 	}
 
@@ -103,6 +171,8 @@ impl RuntimeCache {
 	///
 	/// `allow_missing_func_imports` - Ignore missing function imports.
 	///
+	/// `max_runtime_instances` - The size of the instances cache.
+	///
 	/// `f` - Function to execute.
 	///
 	/// # Returns result of `f` wrapped in an additonal result.
@@ -154,6 +224,7 @@ impl RuntimeCache {
 					heap_pages,
 					host_functions.into(),
 					allow_missing_func_imports,
+					self.max_runtime_instances,
 				);
 				if let Err(ref err) = result {
 					log::warn!(target: "wasm-runtime", "Cannot create a runtime: {:?}", err);
@@ -179,53 +250,7 @@ impl RuntimeCache {
 		}
 		drop(runtimes);
 
-		let result = {
-			// Find a free instance
-			let instance_pool = runtime.instances.read().clone();
-			let instance = instance_pool
-				.iter()
-				.find_map(|i| i.as_ref().and_then(|i| i.try_lock()));
-			if let Some(mut locked) = instance {
-				let result = f(&**locked, runtime.version.as_ref(), ext);
-				if let Err(e) = &result {
-					log::warn!(target: "wasm-runtime", "Evicting failed runtime instance: {:?}", e);
-					*locked = runtime.module.new_instance()?;
-				}
-				result
-			} else {
-				// Allocate a new instance
-				let instance = runtime.module.new_instance()?;
-
-				let result = f(&*instance, runtime.version.as_ref(), ext);
-				match &result {
-					Ok(_) => {
-						let mut instance_pool = runtime.instances.write();
-						if let Some(ref mut slot) = instance_pool.iter_mut().find(|s| s.is_none()) {
-							**slot = Some(Arc::new(Mutex::new(instance)));
-							log::debug!(
-								target: "wasm-runtime",
-								"Allocated WASM instance {}/{}",
-								instance_pool.len(),
-								MAX_INSTANCES,
-							);
-						} else {
-							log::warn!(target: "wasm-runtime", "Ran out of free WASM instances");
-						}
-					}
-					Err(e) => {
-						log::warn!(
-							target:
-							"wasm-runtime",
-							"Fresh runtime instance failed with {:?}",
-							e,
-						);
-					}
-				}
-				result
-			}
-		};
-
-		Ok(result)
+		Ok(runtime.with_instance(ext, f))
 	}
 }
 
@@ -264,6 +289,7 @@ fn create_versioned_wasm_runtime(
 	heap_pages: u64,
 	host_functions: Vec<&'static dyn Function>,
 	allow_missing_func_imports: bool,
+	max_instances: usize,
 ) -> Result<VersionedRuntime, WasmError> {
 	#[cfg(not(target_os = "unknown"))]
 	let time = std::time::Instant::now();
@@ -303,13 +329,16 @@ fn create_versioned_wasm_runtime(
 		time.elapsed().as_millis(),
 	);
 
+	let mut instances = Vec::with_capacity(max_instances);
+	instances.resize_with(max_instances, || Mutex::new(None));
+
 	Ok(VersionedRuntime {
 		code_hash,
 		module: runtime,
 		version,
 		heap_pages,
 		wasm_method,
-		instances: Default::default(),
+		instances,
 	})
 }
 

client/finality-grandpa/src/communication/tests.rs

@@ -482,7 +482,7 @@ fn peer_with_higher_view_leads_to_catch_up_request() {
 		.map(move |tester| {
 			// register a peer with authority role.
 			tester.gossip_validator.new_peer(&mut NoopContext, &id, sc_network::config::Roles::AUTHORITY);
-			((tester, id))
+			(tester, id)
 		})
 		.then(move |(tester, id)| {
 			// send neighbor message at round 10 and height 50