Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve Runtime execution by caching runtime lookup #276

Merged
merged 4 commits into from
Jul 5, 2018
Merged

Improve Runtime execution by caching runtime lookup #276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bae0f5d
Improve Runtime execution by caching runtime lookup
gnunicorn Jul 4, 2018
e97441b
Additional comment and code style fixes
gnunicorn Jul 4, 2018
45a58ca
Fallback to WASM runtime if we can't call the version function
gnunicorn Jul 4, 2018
aa88a45
The previous assumption that the wasm-code was compiled with rustc mi…
gnunicorn Jul 4, 2018
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
3 changes: 3 additions & 0 deletions Cargo.lock
View file
Open in desktop

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

3 changes: 3 additions & 0 deletions substrate/executor/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ byteorder = "1.1"
rustc-hex = "1.0.0"
triehash = "0.1.0"
hex-literal = "0.1.0"
twox-hash = "1.1.0"
lazy_static = "1.0"
parking_lot = "*"
log = "0.3"

[dev-dependencies]
Expand Down
5 changes: 5 additions & 0 deletions substrate/executor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,13 @@ extern crate wasmi;
extern crate byteorder;
extern crate rustc_hex;
extern crate triehash;
extern crate parking_lot;
extern crate twox_hash;
#[macro_use] extern crate log;

#[macro_use]
extern crate lazy_static;

#[macro_use]
extern crate error_chain;

Expand Down
57 changes: 46 additions & 11 deletions substrate/executor/src/native_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,32 @@
use error::{Error, ErrorKind, Result};
use state_machine::{CodeExecutor, Externalities};
use wasm_executor::WasmExecutor;
use wasmi::Module as WasmModule;
use runtime_version::RuntimeVersion;
use std::collections::HashMap;
use codec::Slicable;
use twox_hash::XxHash;
use std::hash::Hasher;
use parking_lot::Mutex;
use RuntimeInfo;

// For the internal Runtime Cache
enum RunWith {
NativeRuntime,
WasmRuntime(WasmModule)
}

lazy_static! {
static ref RUNTIMES_CACHE: Mutex<HashMap<u64, RunWith>> = Mutex::new(HashMap::new());
}

// helper function to generate low-over-head caching_keys
fn gen_cache_key(code: &[u8]) -> u64 {
let mut h = XxHash::with_seed(0);
Copy link
Member

Choose a reason for hiding this comment

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

since XxHash is not crypto-secure, it's worth putting a comment here about why we don't consider it possible that two different code preimages will be able to hash to the same u64. Something like "it is asserted that part of the audit process that any potential on-chain code change will have done is to ensure that the two-x hash is different to that of any other :code value from the same chain".

h.write(code);
h.finish()
}

fn safe_call<F, U>(f: F) -> Result<U>
where F: ::std::panic::UnwindSafe + FnOnce() -> U
{
Expand Down Expand Up @@ -60,6 +82,11 @@ pub struct NativeExecutor<D: NativeExecutionDispatch + Sync + Send> {
impl<D: NativeExecutionDispatch + Sync + Send> NativeExecutor<D> {
/// Create new instance.
pub fn new() -> Self {
// FIXME: set this entry at compile time
RUNTIMES_CACHE.lock().insert(
gen_cache_key(D::native_equivalent()),
RunWith::NativeRuntime);

NativeExecutor {
_dummy: Default::default(),
}
Expand Down Expand Up @@ -88,17 +115,25 @@ impl<D: NativeExecutionDispatch + Sync + Send> CodeExecutor for NativeExecutor<D
method: &str,
data: &[u8],
) -> Result<Vec<u8>> {
if code == D::native_equivalent() {
// call native
D::dispatch(ext, method, data)
} else {
let version = WasmExecutor.call(ext, code, "version", &[])?;
let version = RuntimeVersion::decode(&mut version.as_slice());
if version.map_or(false, |v| D::VERSION.can_call_with(&v)) {
return D::dispatch(ext, method, data)
}
// call into wasm.
WasmExecutor.call(ext, code, method, data)
match RUNTIMES_CACHE.lock().entry(gen_cache_key(code)).or_insert_with(|| {
Copy link
Member

Choose a reason for hiding this comment

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

prefer less complexity and indentation at the cost of more lines, like:

let r = RUNTIMES_CACHE.lock()
	.entry(gen_cache_key(code))
	.or_insert_with(|| {
		let wasm_module = WasmModule::from_buffer(code)
			.expect("all modules compiled with rustc are valid wasm code; qed");

		match WasmExecutor.call_in_wasm_module(ext, &wasm_module, "version", &[]) {
			Ok(version) => {
				let version = RuntimeVersion::decode(&mut version.as_slice());
				if version.map_or(false, |v| D::VERSION.can_call_with(&v)) {
					RunWith::NativeRuntime
				} else {
					RunWith::WasmRuntime(wasm_module)
				}
			},
			// broken or old runtime, run with native
			_ => RunWith::NativeRuntime
		}
	});
match r {
	RunWith::NativeRuntime => D::dispatch(ext, method, data),
	RunWith::WasmRuntime(module) => WasmExecutor.call_in_wasm_module(ext, &module, method, data)
}

let wasm_module = WasmModule::from_buffer(code)
.expect("all modules compiled with rustc are valid wasm code; qed");
Copy link
Member

Choose a reason for hiding this comment

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

indent of run-on lines should be one more than original

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have a written style guide somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/paritytech/polkadot/wiki/Style-Guide


match WasmExecutor.call_in_wasm_module(ext, &wasm_module, "version", &[]) {
Ok(version) => {
let version = RuntimeVersion::decode(&mut version.as_slice());
if version.map_or(false, |v| D::VERSION.can_call_with(&v)) {
RunWith::NativeRuntime
} else {
RunWith::WasmRuntime(wasm_module)
}
},
// broken or old runtime, run with native
_ => RunWith::NativeRuntime
}
}) {
RunWith::NativeRuntime => D::dispatch(ext, method, data),
RunWith::WasmRuntime(module) => WasmExecutor.call_in_wasm_module(ext, &module, method, data)
}
}
}
Expand Down
27 changes: 20 additions & 7 deletions substrate/executor/src/wasm_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,21 +468,19 @@ impl_function_executor!(this: FunctionExecutor<'e, E>,
#[derive(Debug, Default, Clone)]
pub struct WasmExecutor;

impl CodeExecutor for WasmExecutor {
type Error = Error;
impl WasmExecutor {

fn call<E: Externalities>(
/// Call a given method in the given wasm-module runtime.
pub fn call_in_wasm_module<E: Externalities>(
&self,
ext: &mut E,
code: &[u8],
module: &Module,
method: &str,
data: &[u8],
) -> Result<Vec<u8>> {
let module = Module::from_buffer(code).expect("all modules compiled with rustc are valid wasm code; qed");

// start module instantiation. Don't run 'start' function yet.
let intermediate_instance = ModuleInstance::new(
&module,
module,
&ImportsBuilder::new()
.with_resolver("env", FunctionExecutor::<E>::resolver())
)?;
Expand Down Expand Up @@ -529,6 +527,21 @@ impl CodeExecutor for WasmExecutor {
}
}

impl CodeExecutor for WasmExecutor {
type Error = Error;

fn call<E: Externalities>(
&self,
ext: &mut E,
code: &[u8],
method: &str,
data: &[u8],
) -> Result<Vec<u8>> {
let module = Module::from_buffer(code).expect("all modules compiled with rustc are valid wasm code; qed");
Copy link
Member

Choose a reason for hiding this comment

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

probably an invalid expectation now, since the code is coming from chain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken from the previous code base.

So we expect this to sort-of happen? What do we want to do in that case then? Have the result come back with a custom Error? How do expect to recover from invalid-wasm-on-chain (like at all?)?

Can make that a custom error (or error_chain this one), but unsure if this shouldn't be part of a specific PR for that then...

Copy link
Member

Choose a reason for hiding this comment

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

we can't really recover in the context of this particular block/state, but we shouldn't panic all the same. should basically just be a "invalid block" that can never be built on and we hope that the consensus algorithm eventually provides us another.

Copy link
Member

Choose a reason for hiding this comment

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

doesn't need to be done in this PR though since as you said it was already there. worth making an issue/TODO note in the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace this case with a ?, however our WasmExecutor relies on rustc-specific assumptions on quite a few occasions. Not sure we can address those easily without defining/creating some spec about what the features the wasm-code has to offer to be compatible.

self.call_in_wasm_module(ext, &module, method, data)
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down