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

Allow the allocator to track the heap changes. #9291

Merged
5 commits merged into from
Jul 7, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
5 changes: 3 additions & 2 deletions client/allocator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@ targets = ["x86_64-unknown-linux-gnu"]
sp-std = { version = "3.0.0", path = "../../primitives/std", default-features = false }
sp-core = { version = "3.0.0", path = "../../primitives/core", default-features = false }
sp-wasm-interface = { version = "3.0.0", path = "../../primitives/wasm-interface", default-features = false }
log = { version = "0.4.11", optional = true }
log = { version = "0.4.11" }
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
thiserror = { version = "1.0.21" }

[features]
default = [ "std" ]
track-heap = []
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
std = [
"sp-std/std",
"sp-core/std",
"sp-wasm-interface/std",
"log",
"log/std",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"log/std",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh about this: now the allocator is even in client. Should this crate really have a separate "std" feature? I can just remove that.

Copy link
Contributor

Choose a reason for hiding this comment

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

The std feature is not needed. We should remove it IMO

]
41 changes: 31 additions & 10 deletions client/allocator/src/freeing_bump.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,9 @@ fn error(msg: &'static str) -> Error {
/// A custom "trace" implementation that is only activated when `feature = std`.
///
/// Uses `wasm-heap` as default target.
macro_rules! trace {
( $( $args:expr ),+ ) => {
sp_std::if_std! {
log::trace!(target: "wasm-heap", $( $args ),+);
}
macro_rules! log {
($level:tt, $( $args:expr ),+ ) => {
log::$level!(target: "wasm-heap", $( $args ),+);
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, this macro looks (at the call sites) a bit weird. Why don't we just remove it altogether, and stick to a regular logging as found in the rest of 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.

this is a patter that I use a lot in pallets, but I will respect the fact that this is the client realm and most logs are not like this, so will revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, my main preference for this is that I don't like repeating target: "..." all over the place.

}
}

Expand Down Expand Up @@ -146,6 +144,7 @@ impl Order {
/// `MIN_POSSIBLE_ALLOCATION <= size <= MAX_POSSIBLE_ALLOCATION`
fn from_size(size: u32) -> Result<Self, Error> {
let clamped_size = if size > MAX_POSSIBLE_ALLOCATION {
log!(warn, "going to fail due to allocating {:?}", size);
return Err(Error::RequestedAllocationTooLarge);
} else if size < MIN_POSSIBLE_ALLOCATION {
MIN_POSSIBLE_ALLOCATION
Expand Down Expand Up @@ -331,6 +330,16 @@ pub struct FreeingBumpHeapAllocator {
free_lists: FreeLists,
total_size: u32,
poisoned: bool,
#[cfg(feature = "track-heap")]
/// Tracks maximum of `(total_size, bumper)`.
tracker: (u32, u32),
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
}

#[cfg(feature = "track-heap")]
impl Drop for FreeingBumpHeapAllocator {
fn drop(&mut self) {
log!(debug, "allocator being destroyed, max total_size {}, max bumper {}", self.tracker.0, self.tracker.1)
}
}

impl FreeingBumpHeapAllocator {
Expand All @@ -347,6 +356,8 @@ impl FreeingBumpHeapAllocator {
free_lists: FreeLists::new(),
total_size: 0,
poisoned: false,
#[cfg(feature = "track-heap")]
tracker: (0, aligned_heap_base),
}
}

Expand Down Expand Up @@ -392,6 +403,7 @@ impl FreeingBumpHeapAllocator {
}
Link::Nil => {
// Corresponding free list is empty. Allocate a new item.
log!(trace, "trying to increment bumper from {} by {} ", self.bumper, order.size() + HEADER_SIZE);
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
Self::bump(
&mut self.bumper,
order.size() + HEADER_SIZE,
Expand All @@ -404,7 +416,16 @@ impl FreeingBumpHeapAllocator {
Header::Occupied(order).write_into(mem, header_ptr)?;

self.total_size += order.size() + HEADER_SIZE;
trace!("Heap size is {} bytes after allocation", self.total_size);
log!(trace, "after allocation, total_size = {}, bumper = {}.", self.total_size, self.bumper);

#[cfg(feature = "track-heap")]
if self.total_size > self.tracker.0 {
self.tracker.0 = self.total_size;
}
#[cfg(feature = "track-heap")]
if self.bumper > self.tracker.1 {
self.tracker.1 = self.bumper;
}
emostov marked this conversation as resolved.
Show resolved Hide resolved

bomb.disarm();
Ok(Pointer::new(header_ptr + HEADER_SIZE))
Expand Down Expand Up @@ -442,19 +463,19 @@ impl FreeingBumpHeapAllocator {
.total_size
.checked_sub(order.size() + HEADER_SIZE)
.ok_or_else(|| error("Unable to subtract from total heap size without overflow"))?;
trace!("Heap size is {} bytes after deallocation", self.total_size);
log!(trace, "after deallocation, total_size = {}, bumper = {}.", self.total_size, self.bumper);

bomb.disarm();
Ok(())
}

/// Increases the `bumper` by `size`.
///
/// Returns the `bumper` from before the increase.
/// Returns an `Error::AllocatorOutOfSpace` if the operation
/// would exhaust the heap.
/// Returns the `bumper` from before the increase. Returns an `Error::AllocatorOutOfSpace` if
/// the operation would exhaust the heap.
fn bump(bumper: &mut u32, size: u32, heap_end: u32) -> Result<u32, Error> {
if *bumper + size > heap_end {
log!(error, "running out of space with current bumper {}, mem size {}", bumper, heap_end);
return Err(Error::AllocatorOutOfSpace);
}

Expand Down
3 changes: 3 additions & 0 deletions client/executor/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,6 @@ wasmtime = [
wasmi-errno = [
"wasmi/errno"
]
track-heap = [
"sc-executor-common/track-heap"
]
1 change: 1 addition & 0 deletions client/executor/common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,4 @@ thiserror = "1.0.21"

[features]
default = []
track-heap = [ "sc-allocator/track-heap" ]
2 changes: 1 addition & 1 deletion client/executor/wasmtime/src/instance_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ fn get_table(instance: &Instance) -> Option<Table> {
.cloned()
}

/// Functions realted to memory.
/// Functions related to memory.
impl InstanceWrapper {
/// Read data from a slice of memory into a destination buffer.
///
Expand Down
2 changes: 1 addition & 1 deletion utils/frame/try-runtime/cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ structopt = "0.3.8"

sc-service = { version = "0.9.0", default-features = false, path = "../../../../client/service" }
sc-cli = { version = "0.9.0", path = "../../../../client/cli" }
sc-executor = { version = "0.9.0", path = "../../../../client/executor" }
sc-executor = { features = ["track-heap"], version = "0.9.0", path = "../../../../client/executor" }
sc-client-api = { version = "3.0.0", path = "../../../../client/api" }
sc-chain-spec = { version = "3.0.0", path = "../../../../client/chain-spec" }
sp-state-machine = { version = "0.9.0", path = "../../../../primitives/state-machine" }
Expand Down