Prevent double allocation of the payload when calling sp_io::storage::get
#11523
Conversation
koute
added
A0-please_review
|
Converted the PR to a draft until the |
| /// Memory allocation stats gathered during the lifetime of the allocator. | ||
| #[derive(Clone, Debug, Default)] | ||
| #[non_exhaustive] | ||
| pub struct AllocationStats { |
There was a problem hiding this comment.
We should also track the maximum single allocation that ever happened. Maybe we can directly track this as percentage of the maximum allowed allocation?
(order.size() + HEADER_SIZE) / MAX_POSSIBLE_ALLOCATION
There was a problem hiding this comment.
Yeah, we could do that, although:
-
personally I'd probably just track its value directly instead of a percentage (e.g. imagine we hook this data to Prometheus/Grafana and we'll change the maximum possible value in the future - we can then have a situation where some nodes can have vastly different maximum allocation sizes depending on which version of the code they're running, and those will be reported exactly the same in the stats; I'd rather just add a field with the
max_possible_allocationto this struct and the percentage can be later calculated when displaying this data) -
there are also some other metrics I'd like to add (e.g. how many allocations were made), but I simply omitted those since this is not yet the allocation stats PR (as I've said in the other comment this is just a preliminary implementation that isn't supposed to be complete), so I'd suggest we do this in a future PR (although if you want I can add this right now; its utility will be limited though so I'm not sure there's much point?)
|
|
||
| /// 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( |
There was a problem hiding this comment.
Are you asking why it's returning an Option? Or why isn't there just a single function instead of two where first variant returning the stats and the second not returning them?
(I assume that it's the latter, but I'll explain anyway for other people who might be reading.)
It is an Option because in theory there might not be any stats to return if we couldn't call into the runtime at all (e.g. because the passed export_name is missing). Otherwise we essentially always want to return those stats: we obviously want to do it when it returns an Ok, but we also want to do it when it returns an Err (e.g. the runtime panicks due to running out of memory).
So why have two functions? Mostly to reduce churn in this PR. This is not the allocation stats PR; I just preemptively added this so that I can write a reasonable test for the issue I'm fixing here. I intend to flesh out the allocation stats (and probably rethink the interface) in a future PR.
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
| /// An implementation of freeing bump allocator. | ||
| /// | ||
| /// Refer to the module-level documentation for further details. | ||
| pub struct FreeingBumpHeapAllocator { | ||
| original_heap_base: u32, |
There was a problem hiding this comment.
Nit (feel free to ignore)
I know that currently this allocator is only used by wasmi and wasmtime which are currently using wasm32 modules.
But the allocator itself looks like it is designed to be generic, maybe makes sense to port all these u32 types to usize?
There was a problem hiding this comment.
Hm... well, I don't think that would be worth it currently. It'd just make the whole thing more complicated for no real gain, and besides, this allocator was designed to be used with WASM if you read some of the comments (although yes, you're right, you could just use it as-is for something else).
If we ever decide to support wasm64-unknown-unknown in the very far future then we'll have to do something with this allocator, but I don't think that day will come anytime soon.
…nt_storage_access
…nt_storage_access
|
Sorry, I didn't realize this needs companion PRs. I've added them both for Polkadot and Cumulus; fortunately they're pretty trivial. |
…nt_storage_access
…nt_storage_access
…nt_storage_access
…nt_storage_access
…nt_storage_access
…nt_storage_access
|
bot merge |
…::get` (paritytech#11523) * Expose allocation stats in `FreeingBumpHeapAllocator` * Return allocation stats when calling into the runtime * Bump `parity-scale-codec` to 3.1.3 (fork) * Prevent double allocation of the payload when calling `sp_io::storage::get` * Fix tests * Remove unnecessary `mut` * Enable the `bytes` feature for `parity-scale-codec` in `sp-runtime-interface` * Update client/allocator/src/freeing_bump.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Bump `parity-scale-codec` to 3.1.3 * Fix some of the UI tests Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
…::get` (paritytech#11523) * Expose allocation stats in `FreeingBumpHeapAllocator` * Return allocation stats when calling into the runtime * Bump `parity-scale-codec` to 3.1.3 (fork) * Prevent double allocation of the payload when calling `sp_io::storage::get` * Fix tests * Remove unnecessary `mut` * Enable the `bytes` feature for `parity-scale-codec` in `sp-runtime-interface` * Update client/allocator/src/freeing_bump.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Bump `parity-scale-codec` to 3.1.3 * Fix some of the UI tests Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
polkadot companion: paritytech/polkadot#5702
cumulus companion: paritytech/cumulus#1390
This PR fixes the issue which @kianenigma found in #11454.
See the comment in the test I've added in
primitives/runtime-interface/test/src/lib.rsfor a more detailed explanation of the issue.I've also added a way to grab runtime memory allocation stats after calling into the runtime. This functionality will be expanded upon as part of the paritytech/polkadot-sdk#44 in the future; for now I've added it just so that I can write a reasonable test for this issue.
This requires the following PR to be merged into
parity-scale-codec: paritytech/parity-scale-codec#342