From e5e94b1c9fb2ecd11b3636d858a741ecc985c590 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Wed, 25 May 2022 13:24:54 +0000 Subject: [PATCH 01/10] Expose allocation stats in `FreeingBumpHeapAllocator` --- client/allocator/src/freeing_bump.rs | 97 ++++++++++++++++------------ client/allocator/src/lib.rs | 2 +- 2 files changed, 57 insertions(+), 42 deletions(-) diff --git a/client/allocator/src/freeing_bump.rs b/client/allocator/src/freeing_bump.rs index 79d6fca6f91b6..52226932fb8fb 100644 --- a/client/allocator/src/freeing_bump.rs +++ b/client/allocator/src/freeing_bump.rs @@ -314,27 +314,50 @@ impl IndexMut for FreeLists { } } +/// Memory allocation stats gathered during the lifetime of the allocator. +#[derive(Clone, Debug, Default)] +#[non_exhaustive] +pub struct AllocationStats { + /// The current number of bytes allocated. + /// + /// This represents how many bytes are allocated *right now*. + pub bytes_allocated: u32, + + /// The peak number of bytes ever allocated. + /// + /// This is the maximum the `bytes_allocated` ever reached. + pub bytes_allocated_peak: u32, + + /// The sum of every allocation ever made. + /// + /// This increases every time a new allocation is made. + pub bytes_allocated_sum: u32, + + /// The amount of address space (in bytes) used by the allocator. + /// + /// This is calculated as the difference between the allocator's bumper + /// and the heap base. + /// + /// Currently the bumper's only ever incremented, so this is simultaneously + /// the current value as well as the peak value. + pub address_space_used: u32, +} + /// An implementation of freeing bump allocator. /// /// Refer to the module-level documentation for further details. pub struct FreeingBumpHeapAllocator { + original_heap_base: u32, bumper: u32, free_lists: FreeLists, - total_size: u32, poisoned: bool, - max_total_size: u32, - max_bumper: u32, last_observed_memory_size: u32, + stats: AllocationStats, } impl Drop for FreeingBumpHeapAllocator { fn drop(&mut self) { - log::debug!( - target: LOG_TARGET, - "allocator being destroyed, max_total_size {}, max_bumper {}", - self.max_total_size, - self.max_bumper, - ) + log::debug!(target: LOG_TARGET, "allocator dropped: {:?}", self.stats) } } @@ -348,13 +371,12 @@ impl FreeingBumpHeapAllocator { let aligned_heap_base = (heap_base + ALIGNMENT - 1) / ALIGNMENT * ALIGNMENT; FreeingBumpHeapAllocator { + original_heap_base: aligned_heap_base, bumper: aligned_heap_base, free_lists: FreeLists::new(), - total_size: 0, poisoned: false, - max_total_size: 0, - max_bumper: aligned_heap_base, last_observed_memory_size: 0, + stats: AllocationStats::default(), } } @@ -412,22 +434,13 @@ impl FreeingBumpHeapAllocator { // Write the order in the occupied header. Header::Occupied(order).write_into(mem, header_ptr)?; - self.total_size += order.size() + HEADER_SIZE; - - log::trace!( - target: LOG_TARGET, - "after allocation, total_size = {}, bumper = {}.", - self.total_size, - self.bumper, - ); + self.stats.bytes_allocated += order.size() + HEADER_SIZE; + self.stats.bytes_allocated_sum += order.size() + HEADER_SIZE; + self.stats.bytes_allocated_peak = + std::cmp::max(self.stats.bytes_allocated_peak, self.stats.bytes_allocated); + self.stats.address_space_used = self.bumper - self.original_heap_base; - // update trackers if needed. - if self.total_size > self.max_total_size { - self.max_total_size = self.total_size; - } - if self.bumper > self.max_bumper { - self.max_bumper = self.bumper; - } + log::trace!(target: LOG_TARGET, "after allocation: {:?}", self.stats); bomb.disarm(); Ok(Pointer::new(header_ptr + HEADER_SIZE)) @@ -469,21 +482,23 @@ impl FreeingBumpHeapAllocator { let prev_head = self.free_lists.replace(order, Link::Ptr(header_ptr)); Header::Free(prev_head).write_into(mem, header_ptr)?; - // Do the total_size book keeping. - self.total_size = self - .total_size + self.stats.bytes_allocated = self + .stats + .bytes_allocated .checked_sub(order.size() + HEADER_SIZE) - .ok_or_else(|| error("Unable to subtract from total heap size without overflow"))?; - log::trace!( - "after deallocation, total_size = {}, bumper = {}.", - self.total_size, - self.bumper, - ); + .ok_or_else(|| error("underflow of the currently allocated bytes count"))?; + + log::trace!("after deallocation: {:?}", self.stats,); bomb.disarm(); Ok(()) } + /// Returns the allocation stats for this allocator. + pub fn stats(&self) -> AllocationStats { + self.stats.clone() + } + /// Increases the `bumper` by `size`. /// /// Returns the `bumper` from before the increase. Returns an `Error::AllocatorOutOfSpace` if @@ -791,13 +806,13 @@ mod tests { let ptr1 = heap.allocate(&mut mem[..], 32).unwrap(); assert_eq!(ptr1, to_pointer(HEADER_SIZE)); heap.deallocate(&mut mem[..], ptr1).expect("failed freeing ptr1"); - assert_eq!(heap.total_size, 0); + assert_eq!(heap.stats.bytes_allocated, 0); assert_eq!(heap.bumper, 40); let ptr2 = heap.allocate(&mut mem[..], 16).unwrap(); assert_eq!(ptr2, to_pointer(48)); heap.deallocate(&mut mem[..], ptr2).expect("failed freeing ptr2"); - assert_eq!(heap.total_size, 0); + assert_eq!(heap.stats.bytes_allocated, 0); assert_eq!(heap.bumper, 64); // when @@ -825,7 +840,7 @@ mod tests { heap.allocate(&mut mem[..], 9).unwrap(); // then - assert_eq!(heap.total_size, HEADER_SIZE + 16); + assert_eq!(heap.stats.bytes_allocated, HEADER_SIZE + 16); } #[test] @@ -840,7 +855,7 @@ mod tests { heap.deallocate(&mut mem[..], ptr).unwrap(); // then - assert_eq!(heap.total_size, 0); + assert_eq!(heap.stats.bytes_allocated, 0); } #[test] @@ -856,7 +871,7 @@ mod tests { } // then - assert_eq!(heap.total_size, 0); + assert_eq!(heap.stats.bytes_allocated, 0); } #[test] diff --git a/client/allocator/src/lib.rs b/client/allocator/src/lib.rs index 2c3e3b2ae841d..2fe63a1ec392c 100644 --- a/client/allocator/src/lib.rs +++ b/client/allocator/src/lib.rs @@ -26,4 +26,4 @@ mod error; mod freeing_bump; pub use error::Error; -pub use freeing_bump::FreeingBumpHeapAllocator; +pub use freeing_bump::{AllocationStats, FreeingBumpHeapAllocator}; From 94b191fd012c3f91250c15d601fa897baab04adc Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Wed, 25 May 2022 13:38:18 +0000 Subject: [PATCH 02/10] Return allocation stats when calling into the runtime --- client/executor/common/src/wasm_runtime.rs | 17 +++++++- client/executor/src/native_executor.rs | 49 +++++++++++++++++++++- client/executor/wasmi/src/lib.rs | 26 +++++++++++- client/executor/wasmtime/src/host.rs | 6 ++- client/executor/wasmtime/src/runtime.rs | 34 ++++++++++++--- 5 files changed, 120 insertions(+), 12 deletions(-) diff --git a/client/executor/common/src/wasm_runtime.rs b/client/executor/common/src/wasm_runtime.rs index d43ad758b144c..d0cc8926144be 100644 --- a/client/executor/common/src/wasm_runtime.rs +++ b/client/executor/common/src/wasm_runtime.rs @@ -21,6 +21,8 @@ use crate::error::Error; use sp_wasm_interface::Value; +pub use sc_allocator::AllocationStats; + /// A method to be used to find the entrypoint when calling into the runtime /// /// Contains variants on how to resolve wasm function that will be invoked. @@ -78,7 +80,20 @@ pub trait WasmInstance: Send { /// Before execution, instance is reset. /// /// Returns the encoded result on success. - fn call(&mut self, method: InvokeMethod, data: &[u8]) -> Result, Error>; + fn call(&mut self, method: InvokeMethod, data: &[u8]) -> Result, Error> { + self.call_with_allocation_stats(method, data).0 + } + + /// Call a method on this WASM instance. + /// + /// Before execution, instance is reset. + /// + /// Returns the encoded result on success. + fn call_with_allocation_stats( + &mut self, + method: InvokeMethod, + data: &[u8], + ) -> (Result, Error>, Option); /// Call an exported method on this WASM instance. /// diff --git a/client/executor/src/native_executor.rs b/client/executor/src/native_executor.rs index ea060a89c13df..d805949d26fd9 100644 --- a/client/executor/src/native_executor.rs +++ b/client/executor/src/native_executor.rs @@ -37,7 +37,7 @@ use std::{ use codec::{Decode, Encode}; use sc_executor_common::{ runtime_blob::RuntimeBlob, - wasm_runtime::{InvokeMethod, WasmInstance, WasmModule}, + wasm_runtime::{AllocationStats, InvokeMethod, WasmInstance, WasmModule}, }; use sp_core::{ traits::{CodeExecutor, Externalities, RuntimeCode, RuntimeSpawn, RuntimeSpawnExt}, @@ -219,6 +219,47 @@ where allow_missing_host_functions: bool, export_name: &str, call_data: &[u8], + ) -> std::result::Result, Error> { + self.uncached_call_impl( + runtime_blob, + ext, + allow_missing_host_functions, + export_name, + call_data, + &mut None, + ) + } + + /// Same as `uncached_call`, except it also returns allocation statistics. + #[doc(hidden)] // We use this function in tests. + pub fn uncached_call_with_allocation_stats( + &self, + runtime_blob: RuntimeBlob, + ext: &mut dyn Externalities, + allow_missing_host_functions: bool, + export_name: &str, + call_data: &[u8], + ) -> (std::result::Result, Error>, Option) { + let mut allocation_stats = None; + let result = self.uncached_call_impl( + runtime_blob, + ext, + allow_missing_host_functions, + export_name, + call_data, + &mut allocation_stats, + ); + (result, allocation_stats) + } + + fn uncached_call_impl( + &self, + runtime_blob: RuntimeBlob, + ext: &mut dyn Externalities, + allow_missing_host_functions: bool, + export_name: &str, + call_data: &[u8], + allocation_stats_out: &mut Option, ) -> std::result::Result, Error> { let module = crate::wasm_runtime::create_wasm_runtime_with_code::( self.method, @@ -235,10 +276,14 @@ where let mut instance = AssertUnwindSafe(instance); let mut ext = AssertUnwindSafe(ext); let module = AssertUnwindSafe(module); + let mut allocation_stats_out = AssertUnwindSafe(allocation_stats_out); with_externalities_safe(&mut **ext, move || { preregister_builtin_ext(module.clone()); - instance.call_export(export_name, call_data) + let (result, allocation_stats) = + instance.call_with_allocation_stats(export_name.into(), call_data); + **allocation_stats_out = allocation_stats; + result }) .and_then(|r| r) } diff --git a/client/executor/wasmi/src/lib.rs b/client/executor/wasmi/src/lib.rs index 199fe431580ed..e17707e158321 100644 --- a/client/executor/wasmi/src/lib.rs +++ b/client/executor/wasmi/src/lib.rs @@ -29,6 +29,7 @@ use wasmi::{ }; use codec::{Decode, Encode}; +use sc_allocator::AllocationStats; use sc_executor_common::{ error::{Error, MessageWithBacktrace, WasmError}, runtime_blob::{DataSegmentsSnapshot, RuntimeBlob}, @@ -489,6 +490,7 @@ fn call_in_wasm_module( host_functions: Arc>, allow_missing_func_imports: bool, missing_functions: Arc>, + allocation_stats: &mut Option, ) -> Result, Error> { // Initialize FunctionExecutor. let table: Option = module_instance @@ -561,6 +563,8 @@ fn call_in_wasm_module( }, }; + *allocation_stats = Some(function_executor.heap.borrow().stats()); + match result { Ok(Some(I64(r))) => { let (ptr, length) = unpack_ptr_and_len(r as u64); @@ -761,8 +765,13 @@ pub struct WasmiInstance { // `self.instance` unsafe impl Send for WasmiInstance {} -impl WasmInstance for WasmiInstance { - fn call(&mut self, method: InvokeMethod, data: &[u8]) -> Result, Error> { +impl WasmiInstance { + fn call_impl( + &mut self, + method: InvokeMethod, + data: &[u8], + allocation_stats: &mut Option, + ) -> Result, Error> { // We reuse a single wasm instance for multiple calls and a previous call (if any) // altered the state. Therefore, we need to restore the instance to original state. @@ -790,8 +799,21 @@ impl WasmInstance for WasmiInstance { self.host_functions.clone(), self.allow_missing_func_imports, self.missing_functions.clone(), + allocation_stats, ) } +} + +impl WasmInstance for WasmiInstance { + fn call_with_allocation_stats( + &mut self, + method: InvokeMethod, + data: &[u8], + ) -> (Result, Error>, Option) { + let mut allocation_stats = None; + let result = self.call_impl(method, data, &mut allocation_stats); + (result, allocation_stats) + } fn get_global_const(&mut self, name: &str) -> Result, Error> { match self.instance.export_by_name(name) { diff --git a/client/executor/wasmtime/src/host.rs b/client/executor/wasmtime/src/host.rs index 0cb64820fa8a3..773973060a4c2 100644 --- a/client/executor/wasmtime/src/host.rs +++ b/client/executor/wasmtime/src/host.rs @@ -23,7 +23,7 @@ use log::trace; use wasmtime::{Caller, Func, Val}; use codec::{Decode, Encode}; -use sc_allocator::FreeingBumpHeapAllocator; +use sc_allocator::{AllocationStats, FreeingBumpHeapAllocator}; use sc_executor_common::{ error::Result, sandbox::{self, SupervisorFuncIndex}, @@ -66,6 +66,10 @@ impl HostState { pub fn take_panic_message(&mut self) -> Option { self.panic_message.take() } + + pub(crate) fn allocation_stats(&self) -> AllocationStats { + self.allocator.stats() + } } /// A `HostContext` implements `FunctionContext` for making host calls from a Wasmtime diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index 8f6826653f27d..2bdbf555afd6c 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -24,7 +24,7 @@ use crate::{ util, }; -use sc_allocator::FreeingBumpHeapAllocator; +use sc_allocator::{AllocationStats, FreeingBumpHeapAllocator}; use sc_executor_common::{ error::{Result, WasmError}, runtime_blob::{ @@ -184,8 +184,13 @@ pub struct WasmtimeInstance { strategy: Strategy, } -impl WasmInstance for WasmtimeInstance { - fn call(&mut self, method: InvokeMethod, data: &[u8]) -> Result> { +impl WasmtimeInstance { + fn call_impl( + &mut self, + method: InvokeMethod, + data: &[u8], + allocation_stats: &mut Option, + ) -> Result> { match &mut self.strategy { Strategy::LegacyInstanceReuse { ref mut instance_wrapper, @@ -205,7 +210,8 @@ impl WasmInstance for WasmtimeInstance { globals_snapshot.apply(&mut InstanceGlobals { instance: instance_wrapper }); let allocator = FreeingBumpHeapAllocator::new(*heap_base); - let result = perform_call(data, instance_wrapper, entrypoint, allocator); + let result = + perform_call(data, instance_wrapper, entrypoint, allocator, allocation_stats); // Signal to the OS that we are done with the linear memory and that it can be // reclaimed. @@ -219,10 +225,22 @@ impl WasmInstance for WasmtimeInstance { let entrypoint = instance_wrapper.resolve_entrypoint(method)?; let allocator = FreeingBumpHeapAllocator::new(heap_base); - perform_call(data, &mut instance_wrapper, entrypoint, allocator) + perform_call(data, &mut instance_wrapper, entrypoint, allocator, allocation_stats) }, } } +} + +impl WasmInstance for WasmtimeInstance { + fn call_with_allocation_stats( + &mut self, + method: InvokeMethod, + data: &[u8], + ) -> (Result>, Option) { + let mut allocation_stats = None; + let result = self.call_impl(method, data, &mut allocation_stats); + (result, allocation_stats) + } fn get_global_const(&mut self, name: &str) -> Result> { match &mut self.strategy { @@ -735,6 +753,7 @@ fn perform_call( instance_wrapper: &mut InstanceWrapper, entrypoint: EntryPoint, mut allocator: FreeingBumpHeapAllocator, + allocation_stats: &mut Option, ) -> Result> { let (data_ptr, data_len) = inject_input_data(instance_wrapper, &mut allocator, data)?; @@ -748,7 +767,10 @@ fn perform_call( .map(unpack_ptr_and_len); // Reset the host state - instance_wrapper.store_mut().data_mut().host_state = None; + let host_state = instance_wrapper.store_mut().data_mut().host_state.take().expect( + "the host state is always set before calling into WASM so it can't be None here; qed", + ); + *allocation_stats = Some(host_state.allocation_stats()); let (output_ptr, output_len) = ret?; let output = extract_output_data(instance_wrapper, output_ptr, output_len)?; From 339ad2a3e793a9ba19902229a781862cd2f27498 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Wed, 25 May 2022 13:46:24 +0000 Subject: [PATCH 03/10] Bump `parity-scale-codec` to 3.1.3 (fork) --- Cargo.lock | 14 ++++++++------ Cargo.toml | 3 +++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7b1a05b7067e2..45462d157fc0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6441,13 +6441,13 @@ dependencies = [ [[package]] name = "parity-scale-codec" -version = "3.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2a7f3fcf5e45fc28b84dcdab6b983e77f197ec01f325a33f404ba6855afd1070" +version = "3.1.3" +source = "git+https://github.com/koute/parity-scale-codec.git?branch=master_bytes_deserialization#1006e454153ebfc9b44f1ac424ed66f4ba58f7f4" dependencies = [ "arrayvec 0.7.1", "bitvec", "byte-slice-cast", + "bytes", "impl-trait-for-tuples", "parity-scale-codec-derive", "serde", @@ -6455,9 +6455,8 @@ dependencies = [ [[package]] name = "parity-scale-codec-derive" -version = "3.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2c6e626dc84025ff56bf1476ed0e30d10c84d7f89a475ef46ebabee1095a8fba" +version = "3.1.2" +source = "git+https://github.com/koute/parity-scale-codec.git?branch=master_bytes_deserialization#1006e454153ebfc9b44f1ac424ed66f4ba58f7f4" dependencies = [ "proc-macro-crate", "proc-macro2", @@ -9795,6 +9794,7 @@ dependencies = [ name = "sp-io" version = "6.0.0" dependencies = [ + "bytes", "futures", "hash-db", "libsecp256k1", @@ -9953,6 +9953,7 @@ dependencies = [ name = "sp-runtime-interface" version = "6.0.0" dependencies = [ + "bytes", "impl-trait-for-tuples", "parity-scale-codec", "primitive-types", @@ -10002,6 +10003,7 @@ dependencies = [ name = "sp-runtime-interface-test-wasm" version = "2.0.0" dependencies = [ + "bytes", "sp-core", "sp-io", "sp-runtime-interface", diff --git a/Cargo.toml b/Cargo.toml index 41739fe6f1ebc..e881ffd43f738 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -302,3 +302,6 @@ inherits = "release" lto = "fat" # https://doc.rust-lang.org/rustc/codegen-options/index.html#codegen-units codegen-units = 1 + +[patch.crates-io] +parity-scale-codec = { git = "https://github.com/koute/parity-scale-codec.git", branch = "master_bytes_deserialization" } From b78d9e383a4ec08527deace2405feb94e84fad19 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Wed, 25 May 2022 13:45:20 +0000 Subject: [PATCH 04/10] Prevent double allocation of the payload when calling `sp_io::storage::get` --- frame/support/src/storage/unhashed.rs | 2 +- primitives/io/Cargo.toml | 3 +- primitives/io/src/lib.rs | 4 +- primitives/runtime-interface/Cargo.toml | 1 + primitives/runtime-interface/src/pass_by.rs | 8 +-- .../runtime-interface/test-wasm/Cargo.toml | 1 + .../runtime-interface/test-wasm/src/lib.rs | 20 ++++++ primitives/runtime-interface/test/src/lib.rs | 72 +++++++++++++++---- 8 files changed, 88 insertions(+), 23 deletions(-) diff --git a/frame/support/src/storage/unhashed.rs b/frame/support/src/storage/unhashed.rs index adabc9c14a3d5..44bc9db6fa79a 100644 --- a/frame/support/src/storage/unhashed.rs +++ b/frame/support/src/storage/unhashed.rs @@ -102,7 +102,7 @@ pub fn kill_prefix(prefix: &[u8], limit: Option) -> sp_io::KillStorageResul /// Get a Vec of bytes from storage. pub fn get_raw(key: &[u8]) -> Option> { - sp_io::storage::get(key) + sp_io::storage::get(key).map(|value| value.to_vec()) } /// Put a raw byte slice into storage. diff --git a/primitives/io/Cargo.toml b/primitives/io/Cargo.toml index bd288dd896d1b..ced0334cd58a3 100644 --- a/primitives/io/Cargo.toml +++ b/primitives/io/Cargo.toml @@ -15,7 +15,8 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] -codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false } +bytes = { version = "1.1.0", default-features = false } +codec = { package = "parity-scale-codec", version = "3.1.3", default-features = false, features = ["bytes"] } hash-db = { version = "0.15.2", default-features = false } sp-core = { version = "6.0.0", default-features = false, path = "../core" } sp-keystore = { version = "0.12.0", default-features = false, optional = true, path = "../keystore" } diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs index 8f62b03b62ea9..f993592c7a353 100644 --- a/primitives/io/src/lib.rs +++ b/primitives/io/src/lib.rs @@ -109,8 +109,8 @@ pub enum KillStorageResult { #[runtime_interface] pub trait Storage { /// Returns the data for `key` in the storage or `None` if the key can not be found. - fn get(&self, key: &[u8]) -> Option> { - self.storage(key).map(|s| s.to_vec()) + fn get(&self, key: &[u8]) -> Option { + self.storage(key).map(|s| bytes::Bytes::from(s.to_vec())) } /// Get `key` from storage, placing the value into `value_out` and return the number of diff --git a/primitives/runtime-interface/Cargo.toml b/primitives/runtime-interface/Cargo.toml index c9419f73722c6..cd52aded95945 100644 --- a/primitives/runtime-interface/Cargo.toml +++ b/primitives/runtime-interface/Cargo.toml @@ -14,6 +14,7 @@ readme = "README.md" targets = ["x86_64-unknown-linux-gnu"] [dependencies] +bytes = { version = "1.1.0", default-features = false } sp-wasm-interface = { version = "6.0.0", path = "../wasm-interface", default-features = false } sp-std = { version = "4.0.0", default-features = false, path = "../std" } sp-tracing = { version = "5.0.0", default-features = false, path = "../tracing" } diff --git a/primitives/runtime-interface/src/pass_by.rs b/primitives/runtime-interface/src/pass_by.rs index 5d895ff5b3f82..57f333e6b0ee7 100644 --- a/primitives/runtime-interface/src/pass_by.rs +++ b/primitives/runtime-interface/src/pass_by.rs @@ -247,13 +247,13 @@ impl PassByImpl for Codec { let (ptr, len) = unpack_ptr_and_len(arg); let len = len as usize; - let encoded = if len == 0 { - Vec::new() + let mut encoded = if len == 0 { + bytes::Bytes::new() } else { - unsafe { Vec::from_raw_parts(ptr as *mut u8, len, len) } + bytes::Bytes::from(unsafe { Vec::from_raw_parts(ptr as *mut u8, len, len) }) }; - T::decode(&mut &encoded[..]).expect("Host to wasm values are encoded correctly; qed") + codec::decode_from_bytes(encoded).expect("Host to wasm values are encoded correctly; qed") } } diff --git a/primitives/runtime-interface/test-wasm/Cargo.toml b/primitives/runtime-interface/test-wasm/Cargo.toml index f0e78e0e536b9..e9b2937227db6 100644 --- a/primitives/runtime-interface/test-wasm/Cargo.toml +++ b/primitives/runtime-interface/test-wasm/Cargo.toml @@ -13,6 +13,7 @@ publish = false targets = ["x86_64-unknown-linux-gnu"] [dependencies] +bytes = { version = "1.1.0", default-features = false } sp-core = { version = "6.0.0", default-features = false, path = "../../core" } sp-io = { version = "6.0.0", default-features = false, path = "../../io" } sp-runtime-interface = { version = "6.0.0", default-features = false, path = "../" } diff --git a/primitives/runtime-interface/test-wasm/src/lib.rs b/primitives/runtime-interface/test-wasm/src/lib.rs index f518a2e17498c..1305aef66cacc 100644 --- a/primitives/runtime-interface/test-wasm/src/lib.rs +++ b/primitives/runtime-interface/test-wasm/src/lib.rs @@ -60,6 +60,18 @@ pub trait TestApi { vec![0; 4 * 1024] } + fn return_option_vec() -> Option> { + let mut vec = Vec::new(); + vec.resize(16 * 1024, 0xAA); + Some(vec) + } + + fn return_option_bytes() -> Option { + let mut vec = Vec::new(); + vec.resize(16 * 1024, 0xAA); + Some(vec.into()) + } + /// Set the storage at key with value. fn set_storage(&mut self, key: &[u8], data: &[u8]) { self.place_storage(key.to_vec(), Some(data.to_vec())); @@ -300,4 +312,12 @@ wasm_export_functions! { assert_eq!(c, res.2); assert_eq!(d, res.3); } + + fn test_return_option_vec() { + test_api::return_option_vec(); + } + + fn test_return_option_bytes() { + test_api::return_option_bytes(); + } } diff --git a/primitives/runtime-interface/test/src/lib.rs b/primitives/runtime-interface/test/src/lib.rs index 1ab8dbfbbff22..d1db3e064295e 100644 --- a/primitives/runtime-interface/test/src/lib.rs +++ b/primitives/runtime-interface/test/src/lib.rs @@ -23,7 +23,7 @@ use sp_runtime_interface::*; use sp_runtime_interface_test_wasm::{test_api::HostFunctions, wasm_binary_unwrap}; use sp_runtime_interface_test_wasm_deprecated::wasm_binary_unwrap as wasm_binary_deprecated_unwrap; -use sc_executor_common::runtime_blob::RuntimeBlob; +use sc_executor_common::{runtime_blob::RuntimeBlob, wasm_runtime::AllocationStats}; use sp_wasm_interface::{ExtendedHostFunctions, HostFunctions as HostFunctionsT}; use std::{ @@ -36,7 +36,7 @@ type TestExternalities = sp_state_machine::TestExternalities( binary: &[u8], method: &str, -) -> Result { +) -> (Result, Option) { let mut ext = TestExternalities::default(); let mut ext_ext = ext.ext(); @@ -44,20 +44,21 @@ fn call_wasm_method_with_result( ExtendedHostFunctions, >::new(sc_executor::WasmExecutionMethod::Interpreted, Some(8), 8, None, 2); - executor - .uncached_call( - RuntimeBlob::uncompress_if_needed(binary).expect("Failed to parse binary"), - &mut ext_ext, - false, - method, - &[], - ) - .map_err(|e| format!("Failed to execute `{}`: {}", method, e))?; - Ok(ext) + let (result, allocation_stats) = executor.uncached_call_with_allocation_stats( + RuntimeBlob::uncompress_if_needed(binary).expect("Failed to parse binary"), + &mut ext_ext, + false, + method, + &[], + ); + let result = result + .map_err(|e| format!("Failed to execute `{}`: {}", method, e)) + .map(|_| ext); + (result, allocation_stats) } fn call_wasm_method(binary: &[u8], method: &str) -> TestExternalities { - call_wasm_method_with_result::(binary, method).unwrap() + call_wasm_method_with_result::(binary, method).0.unwrap() } #[test] @@ -103,8 +104,9 @@ fn test_return_input_public_key() { #[test] fn host_function_not_found() { - let err = - call_wasm_method_with_result::<()>(wasm_binary_unwrap(), "test_return_data").unwrap_err(); + let err = call_wasm_method_with_result::<()>(wasm_binary_unwrap(), "test_return_data") + .0 + .unwrap_err(); assert!(err.contains("Instantiation: Export ")); assert!(err.contains(" not found")); @@ -236,3 +238,43 @@ fn test_tracing() { fn test_return_input_as_tuple() { call_wasm_method::(wasm_binary_unwrap(), "test_return_input_as_tuple"); } + +#[test] +fn test_returning_option_bytes_from_a_host_function_is_efficient() { + let (result, stats_vec) = call_wasm_method_with_result::( + wasm_binary_unwrap(), + "test_return_option_vec", + ); + result.unwrap(); + let (result, stats_bytes) = call_wasm_method_with_result::( + wasm_binary_unwrap(), + "test_return_option_bytes", + ); + result.unwrap(); + + let stats_vec = stats_vec.unwrap(); + let stats_bytes = stats_bytes.unwrap(); + + // The way we currently implement marshaling of `Option>` through + // the WASM FFI boundary from the host to the runtime requires that it is + // marshaled through SCALE. This is quite inefficient as it requires two + // memory allocations inside of the runtime: + // + // 1) the first allocation to copy the SCALE-encoded blob into the runtime; + // 2) and another allocation for the resulting `Vec` when decoding that blob. + // + // Both of these allocations are are as big as the `Vec` which is being + // passed to the runtime. This is especially bad when fetching big values + // from storage, as it can lead to an out-of-memory situation. + // + // Our `Option` marshaling is better; it still must go through SCALE, + // and it still requires two allocations, however since `Bytes` is zero-copy + // only the first allocation is `Vec`-sized, and the second allocation + // which creates the deserialized `Bytes` is tiny, and is only necessary because + // the underlying `Bytes` buffer from which we're deserializing gets automatically + // turned into an `Arc`. + // + // So this assertion tests that deserializing `Option` allocates less than + // deserializing `Option>`. + assert_eq!(stats_bytes.bytes_allocated_sum + 16 * 1024 + 8, stats_vec.bytes_allocated_sum); +} From 709fc7830e707f88215f47bf8e3d984989cbf90d Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Fri, 3 Jun 2022 08:51:19 +0000 Subject: [PATCH 05/10] Fix tests --- primitives/io/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs index f993592c7a353..0605b96b6a9c0 100644 --- a/primitives/io/src/lib.rs +++ b/primitives/io/src/lib.rs @@ -1700,7 +1700,7 @@ mod tests { t.execute_with(|| { assert_eq!(storage::get(b"hello"), None); storage::set(b"hello", b"world"); - assert_eq!(storage::get(b"hello"), Some(b"world".to_vec())); + assert_eq!(storage::get(b"hello"), Some(b"world".to_vec().into())); assert_eq!(storage::get(b"foo"), None); storage::set(b"foo", &[1, 2, 3][..]); }); @@ -1712,7 +1712,7 @@ mod tests { t.execute_with(|| { assert_eq!(storage::get(b"hello"), None); - assert_eq!(storage::get(b"foo"), Some(b"bar".to_vec())); + assert_eq!(storage::get(b"foo"), Some(b"bar".to_vec().into())); }); let value = vec![7u8; 35]; @@ -1722,7 +1722,7 @@ mod tests { t.execute_with(|| { assert_eq!(storage::get(b"hello"), None); - assert_eq!(storage::get(b"foo00"), Some(value.clone())); + assert_eq!(storage::get(b"foo00"), Some(value.clone().into())); }); } From 68bd3d400edc2c20734fde16d9c08a31ccddc789 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Fri, 3 Jun 2022 11:41:41 +0000 Subject: [PATCH 06/10] Remove unnecessary `mut` --- primitives/runtime-interface/src/pass_by.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/runtime-interface/src/pass_by.rs b/primitives/runtime-interface/src/pass_by.rs index 57f333e6b0ee7..ac6f0def9cad0 100644 --- a/primitives/runtime-interface/src/pass_by.rs +++ b/primitives/runtime-interface/src/pass_by.rs @@ -247,7 +247,7 @@ impl PassByImpl for Codec { let (ptr, len) = unpack_ptr_and_len(arg); let len = len as usize; - let mut encoded = if len == 0 { + let encoded = if len == 0 { bytes::Bytes::new() } else { bytes::Bytes::from(unsafe { Vec::from_raw_parts(ptr as *mut u8, len, len) }) From 29fe671d10411c31a146ceceef17657b0e679f87 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Fri, 3 Jun 2022 12:26:42 +0000 Subject: [PATCH 07/10] Enable the `bytes` feature for `parity-scale-codec` in `sp-runtime-interface` --- primitives/runtime-interface/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/runtime-interface/Cargo.toml b/primitives/runtime-interface/Cargo.toml index 72da43aa44eac..a657c98381c9a 100644 --- a/primitives/runtime-interface/Cargo.toml +++ b/primitives/runtime-interface/Cargo.toml @@ -20,7 +20,7 @@ sp-std = { version = "4.0.0", default-features = false, path = "../std" } sp-tracing = { version = "5.0.0", default-features = false, path = "../tracing" } sp-runtime-interface-proc-macro = { version = "5.0.0", path = "proc-macro" } sp-externalities = { version = "0.12.0", default-features = false, path = "../externalities" } -codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false } +codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["bytes"] } static_assertions = "1.0.0" primitive-types = { version = "0.11.1", default-features = false } sp-storage = { version = "6.0.0", default-features = false, path = "../storage" } From c062a9b9c4eca5f7f482b446afd6bb3decfc2e82 Mon Sep 17 00:00:00 2001 From: Koute Date: Fri, 10 Jun 2022 17:52:51 +0900 Subject: [PATCH 08/10] Update client/allocator/src/freeing_bump.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/allocator/src/freeing_bump.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/allocator/src/freeing_bump.rs b/client/allocator/src/freeing_bump.rs index 52226932fb8fb..13dc6dca0dcd6 100644 --- a/client/allocator/src/freeing_bump.rs +++ b/client/allocator/src/freeing_bump.rs @@ -488,7 +488,7 @@ impl FreeingBumpHeapAllocator { .checked_sub(order.size() + HEADER_SIZE) .ok_or_else(|| error("underflow of the currently allocated bytes count"))?; - log::trace!("after deallocation: {:?}", self.stats,); + log::trace!("after deallocation: {:?}", self.stats); bomb.disarm(); Ok(()) From 3cf7ce1bcb66582c0856463aee898bb71a5abdd1 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Fri, 10 Jun 2022 12:59:29 +0000 Subject: [PATCH 09/10] Bump `parity-scale-codec` to 3.1.3 --- Cargo.lock | 8 +++++--- Cargo.toml | 3 --- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b580a381b74a0..b5bbd82b9ab8d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6461,7 +6461,8 @@ dependencies = [ [[package]] name = "parity-scale-codec" version = "3.1.3" -source = "git+https://github.com/koute/parity-scale-codec.git?branch=master_bytes_deserialization#1006e454153ebfc9b44f1ac424ed66f4ba58f7f4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "04bc9583b5e01cc8c70d89acc9af14ef9b1c29ee3a0074b2a9eea8c0fa396690" dependencies = [ "arrayvec 0.7.1", "bitvec", @@ -6474,8 +6475,9 @@ dependencies = [ [[package]] name = "parity-scale-codec-derive" -version = "3.1.2" -source = "git+https://github.com/koute/parity-scale-codec.git?branch=master_bytes_deserialization#1006e454153ebfc9b44f1ac424ed66f4ba58f7f4" +version = "3.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9299338969a3d2f491d65f140b00ddec470858402f888af98e8642fb5e8965cd" dependencies = [ "proc-macro-crate", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index 336922dc2c0fa..74e7aae7949c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -303,6 +303,3 @@ inherits = "release" lto = "fat" # https://doc.rust-lang.org/rustc/codegen-options/index.html#codegen-units codegen-units = 1 - -[patch.crates-io] -parity-scale-codec = { git = "https://github.com/koute/parity-scale-codec.git", branch = "master_bytes_deserialization" } From 93181549e4b9134ab5daf651ea3ba530510c170a Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Fri, 29 Jul 2022 05:49:55 +0000 Subject: [PATCH 10/10] Fix some of the UI tests --- .../storage_ensure_span_are_ok_on_wrong_gen.stderr | 12 ++++++------ ...ge_ensure_span_are_ok_on_wrong_gen_unnamed.stderr | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen.stderr b/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen.stderr index e1c1b32d65c39..45cdfad67b8ae 100644 --- a/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen.stderr +++ b/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen.stderr @@ -28,7 +28,7 @@ error[E0277]: the trait bound `Bar: EncodeLike` is not satisfied <&[(T,)] as EncodeLike>> <&[(T,)] as EncodeLike>> <&[T] as EncodeLike>> - and 273 others + and 278 others = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar` = note: required because of the requirements on the impl of `PartialStorageInfoTrait` for `frame_support::pallet_prelude::StorageValue<_GeneratedPrefixForStorageFoo, Bar>` @@ -47,8 +47,8 @@ error[E0277]: the trait bound `Bar: WrapperTypeEncode` is not satisfied Cow<'a, T> Rc Vec - frame_support::sp_runtime::sp_application_crypto::sp_core::Bytes - and 2 others + bytes::bytes::Bytes + and 3 others = note: required because of the requirements on the impl of `Encode` for `Bar` = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar` @@ -103,7 +103,7 @@ error[E0277]: the trait bound `Bar: EncodeLike` is not satisfied <&[(T,)] as EncodeLike>> <&[(T,)] as EncodeLike>> <&[T] as EncodeLike>> - and 273 others + and 278 others = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar` = note: required because of the requirements on the impl of `StorageEntryMetadataBuilder` for `frame_support::pallet_prelude::StorageValue<_GeneratedPrefixForStorageFoo, Bar>` @@ -122,8 +122,8 @@ error[E0277]: the trait bound `Bar: WrapperTypeEncode` is not satisfied Cow<'a, T> Rc Vec - frame_support::sp_runtime::sp_application_crypto::sp_core::Bytes - and 2 others + bytes::bytes::Bytes + and 3 others = note: required because of the requirements on the impl of `Encode` for `Bar` = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar` diff --git a/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen_unnamed.stderr b/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen_unnamed.stderr index 7d8c448f00193..d7441e8b18562 100644 --- a/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen_unnamed.stderr +++ b/frame/support/test/tests/pallet_ui/storage_ensure_span_are_ok_on_wrong_gen_unnamed.stderr @@ -28,7 +28,7 @@ error[E0277]: the trait bound `Bar: EncodeLike` is not satisfied <&[(T,)] as EncodeLike>> <&[(T,)] as EncodeLike>> <&[T] as EncodeLike>> - and 273 others + and 278 others = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar` = note: required because of the requirements on the impl of `PartialStorageInfoTrait` for `frame_support::pallet_prelude::StorageValue<_GeneratedPrefixForStorageFoo, Bar>` @@ -47,8 +47,8 @@ error[E0277]: the trait bound `Bar: WrapperTypeEncode` is not satisfied Cow<'a, T> Rc Vec - frame_support::sp_runtime::sp_application_crypto::sp_core::Bytes - and 2 others + bytes::bytes::Bytes + and 3 others = note: required because of the requirements on the impl of `Encode` for `Bar` = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar` @@ -103,7 +103,7 @@ error[E0277]: the trait bound `Bar: EncodeLike` is not satisfied <&[(T,)] as EncodeLike>> <&[(T,)] as EncodeLike>> <&[T] as EncodeLike>> - and 273 others + and 278 others = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar` = note: required because of the requirements on the impl of `StorageEntryMetadataBuilder` for `frame_support::pallet_prelude::StorageValue<_GeneratedPrefixForStorageFoo, Bar>` @@ -122,8 +122,8 @@ error[E0277]: the trait bound `Bar: WrapperTypeEncode` is not satisfied Cow<'a, T> Rc Vec - frame_support::sp_runtime::sp_application_crypto::sp_core::Bytes - and 2 others + bytes::bytes::Bytes + and 3 others = note: required because of the requirements on the impl of `Encode` for `Bar` = note: required because of the requirements on the impl of `FullEncode` for `Bar` = note: required because of the requirements on the impl of `FullCodec` for `Bar`