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

Decommit instance memory after a runtime call on Linux #8998

Merged
12 commits merged into from
Jun 14, 2021
4 changes: 4 additions & 0 deletions Cargo.lock

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

2 changes: 1 addition & 1 deletion bin/node/executor/benches/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<H> = CoreTestExternalities<H, u64>;

Expand Down
30 changes: 30 additions & 0 deletions client/executor/runtime-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,36 @@ 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.
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);
}
}
pepyakin marked this conversation as resolved.
Show resolved Hide resolved

fn round_up_to(n: usize, divisor: usize) -> usize {
(n + divisor - 1) / divisor
}
}

fn test_exhaust_heap() -> Vec<u8> { Vec::with_capacity(16777216) }

fn test_panic() { panic!("test panic") }
Expand Down
82 changes: 82 additions & 0 deletions client/executor/src/integration_tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
};
pepyakin marked this conversation as resolved.
Show resolved Hide resolved
}

fn call_in_wasm<E: Externalities>(
Expand Down Expand Up @@ -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::<u32>()
.expect("failed to parse the RSS");

return rss
}

panic!("smaps doesn't contain rss!");
}

// We test this only under unix because
pepyakin marked this conversation as resolved.
Show resolved Hide resolved
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 */);
}
}
2 changes: 2 additions & 0 deletions client/executor/wasmtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
32 changes: 32 additions & 0 deletions client/executor/wasmtime/src/instance_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need cfg_if here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean that madvise is POSIX-compatible? Sure, but it's complicated. In Linux this is not merely an advice it has a very certain and widely relied semantics of eagerly freeing and zeroing the pages. macOS is, on the other hand, is very lazy about those. madvise(WONTNEED) won't have the same effect there.

There are different options there to make it work on macOS, but those are out of scope for this PR. When the time comes though it will be clear where to integrate them : )

Copy link
Member

Choose a reason for hiding this comment

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

I mean we don't need the cfg_if.

Could be just cfg(unix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Assuming you meant cfg(target_os = "linux"))

Yeah, true, not necessary. I figured I use it because:

  • it's not a new dependency. We already use it and it is a very common dep that it occurs in our dep tree many times
  • as I mentioned it's likely we want other branches for macOS and/or cfg(unix)

It's not something I am married to, so can change to a plain cfg as well. Just let me know

Copy link
Member

Choose a reason for hiding this comment

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

Yeah fine

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 {
Expand Down
8 changes: 7 additions & 1 deletion client/executor/wasmtime/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()?;
Expand Down