From c97a77e57ac5329661ecd4979200611c3fc18676 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Mon, 31 May 2021 16:54:57 +0000 Subject: [PATCH 01/11] Decommit instance memory after a runtime call on Linux --- Cargo.lock | 4 + bin/node/executor/benches/bench.rs | 2 +- client/executor/runtime-test/src/lib.rs | 23 ++++++ client/executor/src/integration_tests/mod.rs | 82 +++++++++++++++++++ client/executor/wasmtime/Cargo.toml | 2 + .../executor/wasmtime/src/instance_wrapper.rs | 32 ++++++++ client/executor/wasmtime/src/runtime.rs | 8 +- 7 files changed, 151 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fc107b0a53bc7..a2122b7b9675f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,5 +1,7 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. +version = 3 + [[package]] name = "Inflector" version = "0.11.4" @@ -7430,6 +7432,8 @@ name = "sc-executor-wasmtime" version = "0.9.0" dependencies = [ "assert_matches", + "cfg-if 1.0.0", + "libc", "log", "parity-scale-codec", "parity-wasm 0.42.2", diff --git a/bin/node/executor/benches/bench.rs b/bin/node/executor/benches/bench.rs index d21aedd1d1849..9f596d74057fa 100644 --- a/bin/node/executor/benches/bench.rs +++ b/bin/node/executor/benches/bench.rs @@ -47,7 +47,7 @@ const TRANSACTION_VERSION: u32 = node_runtime::VERSION.transaction_version; const SPEC_VERSION: u32 = node_runtime::VERSION.spec_version; -const HEAP_PAGES: u64 = 20; +const HEAP_PAGES: u64 = 2048; type TestExternalities = CoreTestExternalities; diff --git a/client/executor/runtime-test/src/lib.rs b/client/executor/runtime-test/src/lib.rs index bfba4ef039395..bd51c1c1fac51 100644 --- a/client/executor/runtime-test/src/lib.rs +++ b/client/executor/runtime-test/src/lib.rs @@ -69,6 +69,29 @@ sp_core::wasm_export_functions! { fn test_empty_return() {} + fn test_dirty_plenty_memory(heap_base: u32, heap_pages: u32) { + let mut heap_ptr = heap_base as usize; + + // Find the next wasm page boundary. + let heap_ptr = round_up_to(heap_ptr, 65536); + + // Make it an actual pointer + let heap_ptr = heap_ptr as *mut u8; + + // Traverse the host pages and make each one dirty + let host_pages = heap_pages as usize * 16; + for i in 0..host_pages { + unsafe { + // technically this is an UB, but there is no way Rust can find this out. + heap_ptr.add(i * 4096).write(0); + } + } + + fn round_up_to(n: usize, divisor: usize) -> usize { + (n + divisor - 1) / divisor + } + } + fn test_exhaust_heap() -> Vec { Vec::with_capacity(16777216) } fn test_panic() { panic!("test panic") } diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index fb39429dfdb24..86631b1143456 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -64,6 +64,15 @@ macro_rules! test_wasm_execution { } } }; + + (compiled_only $method_name:ident) => { + paste::item! { + #[test] + fn [<$method_name _compiled>]() { + $method_name(WasmExecutionMethod::Compiled); + } + } + }; } fn call_in_wasm( @@ -775,3 +784,76 @@ fn panic_in_spawned_instance_panics_on_joining_its_result(wasm_method: WasmExecu assert!(format!("{}", error_result).contains("Spawned task")); } + + +#[cfg(target_os = "linux")] +mod linux { + use super::*; + + /// Gets the rollup of for the current process from procfs. + /// + /// See the description here: https://www.kernel.org/doc/Documentation/ABI/testing/procfs-smaps_rollup + fn obtain_rss() -> u32 { + let smaps_rollup = std::fs::read("/proc/self/smaps_rollup").expect("failed to read smaps"); + let smaps_rollup = String::from_utf8(smaps_rollup).expect("expected proper utf8"); + + for line in smaps_rollup.lines() { + // We are looking for a line that looks something like below. We are interested only in the + // number there. + // + // Rss: 63624 kB + // ^^^^^ + // | + // = our target + // + // we don't even care about the suffix since it appears to be hardcoded to kB. + + let line = match line.strip_prefix("Rss:") { + None => continue, + Some(line) => line, + }; + + let line = match line.strip_suffix("kB") { + None => { + panic!("weird, we expected that the smaps output is always terminated with `kB`"); + } + Some(line) => line, + }; + + let line = line.trim(); + + let rss = line.parse::() + .expect("failed to parse the RSS"); + + return rss + } + + panic!("smaps doesn't contain rss!"); + } + + // We test this only under unix because + test_wasm_execution!(compiled_only memory_consumption); + fn memory_consumption(wasm_method: WasmExecutionMethod) { + let runtime = mk_test_runtime(wasm_method, 1024); + + let instance = runtime.new_instance().unwrap(); + let heap_base = instance.get_global_const("__heap_base") + .expect("`__heap_base` is valid") + .expect("`__heap_base` exists") + .as_i32() + .expect("`__heap_base` is an `i32`"); + + instance.call_export("test_dirty_plenty_memory", &(heap_base as u32, 1u32).encode()).unwrap(); + + let rss_pre = obtain_rss(); + instance.call_export("test_dirty_plenty_memory", &(heap_base as u32, 1024u32).encode()).unwrap(); + let rss_post = obtain_rss(); + + // In case the memory is sucessfully decommited we expect a slight raise of resident memory + // usage. However, in practice this test is running in parallel to other tests and the + // memory usage may be a bit skewed. This makes this test flaky. This problem should be + // excaberated with the number of CPU cores. Empirically, this works just fine on the build + // host (numcpus=128). + assert!(rss_post - rss_pre < 32768 /* kB */); + } +} diff --git a/client/executor/wasmtime/Cargo.toml b/client/executor/wasmtime/Cargo.toml index 591565276a9d8..1e886d15beb18 100644 --- a/client/executor/wasmtime/Cargo.toml +++ b/client/executor/wasmtime/Cargo.toml @@ -13,6 +13,8 @@ readme = "README.md" targets = ["x86_64-unknown-linux-gnu"] [dependencies] +libc = "0.2.90" +cfg-if = "1.0" log = "0.4.8" scoped-tls = "1.0" parity-wasm = "0.42.0" diff --git a/client/executor/wasmtime/src/instance_wrapper.rs b/client/executor/wasmtime/src/instance_wrapper.rs index 381ae993442a2..a72b17a1a92f9 100644 --- a/client/executor/wasmtime/src/instance_wrapper.rs +++ b/client/executor/wasmtime/src/instance_wrapper.rs @@ -415,6 +415,38 @@ impl InstanceWrapper { slice::from_raw_parts_mut(ptr, len) } } + + /// Removes physical backing from the allocated linear memory. This leads to returning the memory + /// back to the system. While the memory is zeroed this is considered as a side-effect and is not + /// relied upon. Thus this function acts as a hint. + pub fn decommit(&self) { + if self.memory.data_size() == 0 { + return; + } + + cfg_if::cfg_if! { + if #[cfg(target_os = "linux")] { + use std::sync::Once; + + unsafe { + let ptr = self.memory.data_ptr(); + let len = self.memory.data_size(); + + // Linux handles MADV_DONTNEED reliably. The result is that the given area + // is unmapped and will be zeroed on the next pagefault. + if libc::madvise(ptr as _, len, libc::MADV_DONTNEED) != 0 { + static LOGGED: Once = Once::new(); + LOGGED.call_once(|| { + log::warn!( + "madvise(MADV_DONTNEED) failed: {}", + std::io::Error::last_os_error(), + ); + }); + } + } + } + } + } } impl runtime_blob::InstanceGlobals for InstanceWrapper { diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index fc45345256d1d..4001d07c5d1d1 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -150,7 +150,13 @@ impl WasmInstance for WasmtimeInstance { globals_snapshot.apply(&**instance_wrapper); let allocator = FreeingBumpHeapAllocator::new(*heap_base); - perform_call(data, Rc::clone(&instance_wrapper), entrypoint, allocator) + let result = perform_call(data, Rc::clone(&instance_wrapper), entrypoint, allocator); + + // Signal to the OS that we are done with the linear memory and that it can be + // reclaimed. + instance_wrapper.decommit(); + + result } Strategy::RecreateInstance(instance_creator) => { let instance_wrapper = instance_creator.instantiate()?; From 74cf151fda5ef6d712c3945f21c887351f559b3e Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 2 Jun 2021 14:33:06 +0000 Subject: [PATCH 02/11] Update documentation for the test --- client/executor/runtime-test/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/client/executor/runtime-test/src/lib.rs b/client/executor/runtime-test/src/lib.rs index bd51c1c1fac51..115683bffa62d 100644 --- a/client/executor/runtime-test/src/lib.rs +++ b/client/executor/runtime-test/src/lib.rs @@ -70,6 +70,13 @@ sp_core::wasm_export_functions! { fn test_empty_return() {} fn test_dirty_plenty_memory(heap_base: u32, heap_pages: u32) { + // This piece of code will dirty multiple pages of memory. The number of pages is given by + // the `heap_pages`. It's unit is a wasm page (64KiB). The first page to be cleared + // is a wasm page that that follows the one that holds the `heap_base` address. + // + // This function dirties the **host** pages. I.e. we dirty 4KiB at a time and it will take + // 16 writes to process a single wasm page. + let mut heap_ptr = heap_base as usize; // Find the next wasm page boundary. From 53e29c993138dee9775cf66a91658837ec3ccec3 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Thu, 3 Jun 2021 15:08:41 +0000 Subject: [PATCH 03/11] Remove unfinished comment --- client/executor/src/integration_tests/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index 86631b1143456..03f241c228584 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -831,7 +831,6 @@ mod linux { panic!("smaps doesn't contain rss!"); } - // We test this only under unix because test_wasm_execution!(compiled_only memory_consumption); fn memory_consumption(wasm_method: WasmExecutionMethod) { let runtime = mk_test_runtime(wasm_method, 1024); From ec3cf69cbab00845859aff32689d7bdb1674377d Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Mon, 7 Jun 2021 16:05:19 +0000 Subject: [PATCH 04/11] Use saturating_sub. Also update the doc comment. --- client/executor/src/integration_tests/mod.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index 03f241c228584..f1db5c64562ee 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -848,11 +848,18 @@ mod linux { instance.call_export("test_dirty_plenty_memory", &(heap_base as u32, 1024u32).encode()).unwrap(); let rss_post = obtain_rss(); - // In case the memory is sucessfully decommited we expect a slight raise of resident memory - // usage. However, in practice this test is running in parallel to other tests and the - // memory usage may be a bit skewed. This makes this test flaky. This problem should be - // excaberated with the number of CPU cores. Empirically, this works just fine on the build - // host (numcpus=128). - assert!(rss_post - rss_pre < 32768 /* kB */); + // In case the memory is sucessfully decommited we expect only a slight increase of resident memory + // usage. + // + // However, in practice the memory usage can either go up and sometimes down (hence + // `saturating_sub` below). This is because this test is run along with other concurrently + // and the other tests may allocate or deallocate memory. This in theory makes this test + // flaky. This problem should be excaberated with the number of CPU cores + // + // However, empirically, this works just fine on the "build host" (numcpus=128) and is able + // reliably to tell if the decommiting really works or not. + // + // That said, if the test fails randomly we should consider turning it off. + assert!(rss_post.saturating_sub(rss_pre) < 32768 /* kB */); } } From 5d07a203269dab07a9404e7e5aecf08cdab40587 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Fri, 11 Jun 2021 15:38:41 +0000 Subject: [PATCH 05/11] Precise RSS tracking in the test Instead of tracking RSS for the whole process we just look at the particular mapping that is associated with the linear memory of the runtime instance --- Cargo.lock | 1 + client/executor/Cargo.toml | 1 + client/executor/common/src/wasm_runtime.rs | 9 ++ .../executor/src/integration_tests/linux.rs | 65 +++++++++++++++ .../src/integration_tests/linux/smaps.rs | 82 +++++++++++++++++++ client/executor/src/integration_tests/mod.rs | 82 +------------------ .../executor/wasmtime/src/instance_wrapper.rs | 5 ++ client/executor/wasmtime/src/runtime.rs | 13 +++ 8 files changed, 179 insertions(+), 79 deletions(-) create mode 100644 client/executor/src/integration_tests/linux.rs create mode 100644 client/executor/src/integration_tests/linux/smaps.rs diff --git a/Cargo.lock b/Cargo.lock index a2122b7b9675f..0c132c9a90c9a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7371,6 +7371,7 @@ dependencies = [ "parity-wasm 0.42.2", "parking_lot 0.11.1", "paste 1.0.4", + "regex", "sc-executor-common", "sc-executor-wasmi", "sc-executor-wasmtime", diff --git a/client/executor/Cargo.toml b/client/executor/Cargo.toml index f9ebfd9bd5de5..06a248357e725 100644 --- a/client/executor/Cargo.toml +++ b/client/executor/Cargo.toml @@ -50,6 +50,7 @@ sc-tracing = { version = "3.0.0", path = "../tracing" } tracing = "0.1.25" tracing-subscriber = "0.2.15" paste = "1.0" +regex = "1" [features] default = [ "std" ] diff --git a/client/executor/common/src/wasm_runtime.rs b/client/executor/common/src/wasm_runtime.rs index cca0d99c4b91c..12ff92a2c607f 100644 --- a/client/executor/common/src/wasm_runtime.rs +++ b/client/executor/common/src/wasm_runtime.rs @@ -93,4 +93,13 @@ pub trait WasmInstance: Send { /// /// This method is only suitable for getting immutable globals. fn get_global_const(&self, name: &str) -> Result, Error>; + + /// **Testing Only**. This function returns the base address of the linear memory. + /// + /// This is meant to be the starting address of the memory mapped area for the linear memory. + /// + /// This function is intended only for a specific test that measures physical memory consumption. + fn linear_memory_base_ptr(&self) -> Option<*const u8> { + None + } } diff --git a/client/executor/src/integration_tests/linux.rs b/client/executor/src/integration_tests/linux.rs new file mode 100644 index 0000000000000..bffa69f510386 --- /dev/null +++ b/client/executor/src/integration_tests/linux.rs @@ -0,0 +1,65 @@ +// This file is part of Substrate. + +// Copyright (C) 2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Tests that are only relevant for Linux + +use crate::WasmExecutionMethod; +use super::mk_test_runtime; +use sc_executor_common::wasm_runtime::WasmModule; +use codec::Encode as _; + +mod smaps; + +use self::smaps::Smaps; + +#[cfg(feature = "wasmtime")] +#[test] +fn memory_consumption_compiled() { + let runtime = mk_test_runtime(WasmExecutionMethod::Compiled, 1024); + + let instance = runtime.new_instance().unwrap(); + let heap_base = instance + .get_global_const("__heap_base") + .expect("`__heap_base` is valid") + .expect("`__heap_base` exists") + .as_i32() + .expect("`__heap_base` is an `i32`"); + + let smaps = Smaps::new(); + + instance + .call_export( + "test_dirty_plenty_memory", + &(heap_base as u32, 1u32).encode(), + ) + .unwrap(); + + // Just assume that the linear memory is backed by a single memory mapping. + let base_addr = instance.linear_memory_base_ptr().unwrap() as usize; + let rss_pre = smaps.get_rss(base_addr).unwrap(); + + instance + .call_export( + "test_dirty_plenty_memory", + &(heap_base as u32, 1024u32).encode(), + ) + .unwrap(); + let rss_post = smaps.get_rss(base_addr).unwrap(); + + assert_eq!(rss_post.saturating_sub(rss_pre), 0); +} diff --git a/client/executor/src/integration_tests/linux/smaps.rs b/client/executor/src/integration_tests/linux/smaps.rs new file mode 100644 index 0000000000000..8088a5a3ea952 --- /dev/null +++ b/client/executor/src/integration_tests/linux/smaps.rs @@ -0,0 +1,82 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! A tool for extracting information about the memory consumption of the current process from +//! the procfs. + +use std::ops::Range; +use std::collections::BTreeMap; + +/// An interface to the /proc/self/smaps +/// +/// See docs about [procfs on kernel.org][procfs] +/// +/// [procfs]: https://www.kernel.org/doc/html/latest/filesystems/proc.html +pub struct Smaps(Vec<(Range, BTreeMap)>); + +impl Smaps { + pub fn new() -> Self { + let regex_start = regex::RegexBuilder::new("^([0-9a-f]+)-([0-9a-f]+)") + .multi_line(true) + .build() + .unwrap(); + let regex_kv = regex::RegexBuilder::new(r#"^([^:]+):\s*(\d+) kB"#) + .multi_line(true) + .build() + .unwrap(); + let smaps = std::fs::read_to_string("/proc/self/smaps").unwrap(); + let boundaries: Vec<_> = regex_start + .find_iter(&smaps) + .map(|matched| matched.start()) + .chain(std::iter::once(smaps.len())) + .collect(); + + let mut output = Vec::new(); + for window in boundaries.windows(2) { + let chunk = &smaps[window[0]..window[1]]; + let caps = regex_start.captures(chunk).unwrap(); + let start = usize::from_str_radix(caps.get(1).unwrap().as_str(), 16).unwrap(); + let end = usize::from_str_radix(caps.get(2).unwrap().as_str(), 16).unwrap(); + + let values = regex_kv + .captures_iter(chunk) + .map(|cap| { + let key = cap.get(1).unwrap().as_str().to_owned(); + let value = cap.get(2).unwrap().as_str().parse().unwrap(); + (key, value) + }) + .collect(); + + output.push((start..end, values)); + } + + Self(output) + } + + fn get_map(&self, addr: usize) -> &BTreeMap { + &self.0 + .iter() + .find(|(range, _)| addr >= range.start && addr < range.end) + .unwrap() + .1 + } + + pub fn get_rss(&self, addr: usize) -> Option { + self.get_map(addr).get("Rss").cloned() + } +} diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index f1db5c64562ee..fc6843849386f 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -15,6 +15,9 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . + +#[cfg(target_os = "linux")] +mod linux; mod sandbox; use std::sync::Arc; @@ -784,82 +787,3 @@ fn panic_in_spawned_instance_panics_on_joining_its_result(wasm_method: WasmExecu assert!(format!("{}", error_result).contains("Spawned task")); } - - -#[cfg(target_os = "linux")] -mod linux { - use super::*; - - /// Gets the rollup of for the current process from procfs. - /// - /// See the description here: https://www.kernel.org/doc/Documentation/ABI/testing/procfs-smaps_rollup - fn obtain_rss() -> u32 { - let smaps_rollup = std::fs::read("/proc/self/smaps_rollup").expect("failed to read smaps"); - let smaps_rollup = String::from_utf8(smaps_rollup).expect("expected proper utf8"); - - for line in smaps_rollup.lines() { - // We are looking for a line that looks something like below. We are interested only in the - // number there. - // - // Rss: 63624 kB - // ^^^^^ - // | - // = our target - // - // we don't even care about the suffix since it appears to be hardcoded to kB. - - let line = match line.strip_prefix("Rss:") { - None => continue, - Some(line) => line, - }; - - let line = match line.strip_suffix("kB") { - None => { - panic!("weird, we expected that the smaps output is always terminated with `kB`"); - } - Some(line) => line, - }; - - let line = line.trim(); - - let rss = line.parse::() - .expect("failed to parse the RSS"); - - return rss - } - - panic!("smaps doesn't contain rss!"); - } - - test_wasm_execution!(compiled_only memory_consumption); - fn memory_consumption(wasm_method: WasmExecutionMethod) { - let runtime = mk_test_runtime(wasm_method, 1024); - - let instance = runtime.new_instance().unwrap(); - let heap_base = instance.get_global_const("__heap_base") - .expect("`__heap_base` is valid") - .expect("`__heap_base` exists") - .as_i32() - .expect("`__heap_base` is an `i32`"); - - instance.call_export("test_dirty_plenty_memory", &(heap_base as u32, 1u32).encode()).unwrap(); - - let rss_pre = obtain_rss(); - instance.call_export("test_dirty_plenty_memory", &(heap_base as u32, 1024u32).encode()).unwrap(); - let rss_post = obtain_rss(); - - // In case the memory is sucessfully decommited we expect only a slight increase of resident memory - // usage. - // - // However, in practice the memory usage can either go up and sometimes down (hence - // `saturating_sub` below). This is because this test is run along with other concurrently - // and the other tests may allocate or deallocate memory. This in theory makes this test - // flaky. This problem should be excaberated with the number of CPU cores - // - // However, empirically, this works just fine on the "build host" (numcpus=128) and is able - // reliably to tell if the decommiting really works or not. - // - // That said, if the test fails randomly we should consider turning it off. - assert!(rss_post.saturating_sub(rss_pre) < 32768 /* kB */); - } -} diff --git a/client/executor/wasmtime/src/instance_wrapper.rs b/client/executor/wasmtime/src/instance_wrapper.rs index a72b17a1a92f9..866dbfb2e2bfc 100644 --- a/client/executor/wasmtime/src/instance_wrapper.rs +++ b/client/executor/wasmtime/src/instance_wrapper.rs @@ -416,6 +416,11 @@ impl InstanceWrapper { } } + /// Returns the pointer to the first byte of the linear memory for this instance. + pub fn base_ptr(&self) -> *const u8 { + self.memory.data_ptr() + } + /// Removes physical backing from the allocated linear memory. This leads to returning the memory /// back to the system. While the memory is zeroed this is considered as a side-effect and is not /// relied upon. Thus this function acts as a hint. diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index 4001d07c5d1d1..5018b11264d71 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -179,6 +179,19 @@ impl WasmInstance for WasmtimeInstance { } } } + + fn linear_memory_base_ptr(&self) -> Option<*const u8> { + match &self.strategy { + Strategy::RecreateInstance(_) => { + // We do not keep the wasm instance around, therefore there is no linear memory + // associated with it. + None + } + Strategy::FastInstanceReuse { + instance_wrapper, .. + } => Some(instance_wrapper.base_ptr()), + } + } } /// Prepare a directory structure and a config file to enable wasmtime caching. From b4ee83547fd7dd57b2e32628aae9b056c497bcd0 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Sat, 12 Jun 2021 10:55:53 +0000 Subject: [PATCH 06/11] Remove unused import --- client/executor/src/integration_tests/linux.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/executor/src/integration_tests/linux.rs b/client/executor/src/integration_tests/linux.rs index bffa69f510386..ac7ea6bbedf32 100644 --- a/client/executor/src/integration_tests/linux.rs +++ b/client/executor/src/integration_tests/linux.rs @@ -20,7 +20,6 @@ use crate::WasmExecutionMethod; use super::mk_test_runtime; -use sc_executor_common::wasm_runtime::WasmModule; use codec::Encode as _; mod smaps; @@ -30,6 +29,8 @@ use self::smaps::Smaps; #[cfg(feature = "wasmtime")] #[test] fn memory_consumption_compiled() { + use sc_executor_common::wasm_runtime::WasmModule as _; + let runtime = mk_test_runtime(WasmExecutionMethod::Compiled, 1024); let instance = runtime.new_instance().unwrap(); From 3943a699534af7bae764a6b2ae1ca6cb2302b2bc Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Sat, 12 Jun 2021 12:02:32 +0000 Subject: [PATCH 07/11] Fix unused imports --- client/executor/src/integration_tests/linux.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/client/executor/src/integration_tests/linux.rs b/client/executor/src/integration_tests/linux.rs index ac7ea6bbedf32..c94dc47a5abae 100644 --- a/client/executor/src/integration_tests/linux.rs +++ b/client/executor/src/integration_tests/linux.rs @@ -16,7 +16,12 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! Tests that are only relevant for Linux +//! Tests that are only relevant for Linux. + +// Constrain this only to wasmtime for the time being. Without this rustc will complain on unused +// imports and items. The alternative is to plop `cfg(feature = wasmtime)` everywhere which seems +// borthersome. +#![cfg(feature = "wasmtime")] use crate::WasmExecutionMethod; use super::mk_test_runtime; @@ -26,7 +31,6 @@ mod smaps; use self::smaps::Smaps; -#[cfg(feature = "wasmtime")] #[test] fn memory_consumption_compiled() { use sc_executor_common::wasm_runtime::WasmModule as _; From dfe11702500defb86a33abf1df2568d37b7b6df5 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Sat, 12 Jun 2021 12:40:11 +0000 Subject: [PATCH 08/11] Fix the unused imports error for good --- client/executor/src/integration_tests/linux.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/executor/src/integration_tests/linux.rs b/client/executor/src/integration_tests/linux.rs index c94dc47a5abae..850fe3bae155c 100644 --- a/client/executor/src/integration_tests/linux.rs +++ b/client/executor/src/integration_tests/linux.rs @@ -33,8 +33,6 @@ use self::smaps::Smaps; #[test] fn memory_consumption_compiled() { - use sc_executor_common::wasm_runtime::WasmModule as _; - let runtime = mk_test_runtime(WasmExecutionMethod::Compiled, 1024); let instance = runtime.new_instance().unwrap(); From 75f04d564f95dfdd29f737fdc8df14e6ed68125c Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Mon, 14 Jun 2021 11:00:43 +0100 Subject: [PATCH 09/11] Rollback an accidental change to benches --- bin/node/executor/benches/bench.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/executor/benches/bench.rs b/bin/node/executor/benches/bench.rs index 9f596d74057fa..d21aedd1d1849 100644 --- a/bin/node/executor/benches/bench.rs +++ b/bin/node/executor/benches/bench.rs @@ -47,7 +47,7 @@ const TRANSACTION_VERSION: u32 = node_runtime::VERSION.transaction_version; const SPEC_VERSION: u32 = node_runtime::VERSION.spec_version; -const HEAP_PAGES: u64 = 2048; +const HEAP_PAGES: u64 = 20; type TestExternalities = CoreTestExternalities; From 56e165d360b5a573582ca2e46980b07f346ff2b6 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Mon, 14 Jun 2021 14:45:43 +0000 Subject: [PATCH 10/11] Fix the test --- .../executor/src/integration_tests/linux.rs | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/client/executor/src/integration_tests/linux.rs b/client/executor/src/integration_tests/linux.rs index 850fe3bae155c..057cc1332717b 100644 --- a/client/executor/src/integration_tests/linux.rs +++ b/client/executor/src/integration_tests/linux.rs @@ -33,6 +33,11 @@ use self::smaps::Smaps; #[test] fn memory_consumption_compiled() { + // This aims to see if linear memory stays backed by the physical memory after a runtime call. + // + // For that we make a series of runtime calls, probing the RSS for the VMA matching the linear + // memory. After the call we expect RSS to be equal to 0. + let runtime = mk_test_runtime(WasmExecutionMethod::Compiled, 1024); let instance = runtime.new_instance().unwrap(); @@ -43,7 +48,10 @@ fn memory_consumption_compiled() { .as_i32() .expect("`__heap_base` is an `i32`"); - let smaps = Smaps::new(); + fn probe_rss(instance: &dyn sc_executor_common::wasm_runtime::WasmInstance) -> usize { + let base_addr = instance.linear_memory_base_ptr().unwrap() as usize; + Smaps::new().get_rss(base_addr).expect("failed to get rss") + } instance .call_export( @@ -51,18 +59,15 @@ fn memory_consumption_compiled() { &(heap_base as u32, 1u32).encode(), ) .unwrap(); - - // Just assume that the linear memory is backed by a single memory mapping. - let base_addr = instance.linear_memory_base_ptr().unwrap() as usize; - let rss_pre = smaps.get_rss(base_addr).unwrap(); - + let probe_1 = probe_rss(&*instance); instance .call_export( "test_dirty_plenty_memory", &(heap_base as u32, 1024u32).encode(), ) .unwrap(); - let rss_post = smaps.get_rss(base_addr).unwrap(); + let probe_2 = probe_rss(&*instance); - assert_eq!(rss_post.saturating_sub(rss_pre), 0); + assert_eq!(probe_1, 0); + assert_eq!(probe_2, 0); } From caaa7b528fb7f65c64087e92b3ebfda68809fde9 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Mon, 14 Jun 2021 14:45:51 +0000 Subject: [PATCH 11/11] Remove now unneeded code --- client/executor/src/integration_tests/mod.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index fc6843849386f..8c8674fc3ca95 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -67,15 +67,6 @@ macro_rules! test_wasm_execution { } } }; - - (compiled_only $method_name:ident) => { - paste::item! { - #[test] - fn [<$method_name _compiled>]() { - $method_name(WasmExecutionMethod::Compiled); - } - } - }; } fn call_in_wasm(