wasm-executor: Support growing the memory #12520

bkchr · 2022-10-18T13:51:57Z

This pull request adds support for growing the wasm memory while executing a runtime function. The pull request doesn't change the old default behavior which was to have a fixed amount of heap pages. By default will still only allocate a fixed amount of memory in total for the wasm memory. The new "allocation mode" that is being added is called "dynamic". If this is enabled, the allocator is allowed to grow the wasm memory up to the upper maximum of what wasm32 supports (4GB). Besides these changes there is a new CallContext that needs to passed to the wasm executor call function. Based on this context (onchain/offchain) the executor can use a different heap pages limit as configured on startup. To make it easy to configure the WasmExecutor a new WasmExecutorBuilder is added to simplify the configuration.

Why supporting for different maximum heap pages, based on the context?

I think that on chain logic will always need to stay in certain limits. This should be done to ensure that we don't run into situations where the we allocate huge amounts of data that we want to read in some offchain context. In this offchain context we then fail because all this data combined doesn't fit into the memory. Another example are Parachains, they have a hard cap of 1GB of memory for the validation phase. So, they need to ensure to stay in this limit while building the PoV. However, in the offchain context we can be more free and open up the possibility to allocate up to 4GB, if configured.

Enabling dynamic memory

To enable dynamic memory growing a user would need to use the newly introduced WasmExecutorBuilder and set the appropriate values via with_offchain_heap_alloc_strategy/with_onchain_heap_alloc_strategy. By default nothing changes to the old behavior and everything uses the same memory limits as before. It is also advised to not use this right now, especially not for building/importing blocks. Building/importing blocks should always be done in some fixed limits (especially for Parachains which have only a fixed memory size when validating their blocks) to ensure that everyone can import the blocks.

Works towards: paritytech/polkadot-sdk#62

polkadot companion: paritytech/polkadot#6730

…nto bkchr-grow-memory

Co-authored-by: Koute <[email protected]>

koute

LGTM

client/executor/wasmi/src/lib.rs

michalkucharczyk · 2023-02-21T17:50:09Z

test-utils/runtime/src/lib.rs

@@ -1358,7 +1358,12 @@ mod tests {

 		// Try to allocate 1024k of memory on heap. This is going to fail since it is twice larger
 		// than the heap.
-		let ret = client.runtime_api().vec_with_capacity(&block_id, 1048576);
+		let ret = client.runtime_api().vec_with_capacity_with_context(
+			&block_id,


Will have a conflict here:

Suggested change

&block_id,

client.chain_info().best_hash,

michalkucharczyk · 2023-02-21T17:53:17Z

DQ: Having different memory model for benchmarking (dynamic) and for runtime execution (static) may lead to wrongly estimated cost of memory. For example in case of allocation failure the runtime code may fallback to more expensive algorithm (imagine doing something in place instead of using extra buffer). Not sure if this is applicable here.

davxy · 2023-02-21T17:40:34Z

client/allocator/src/lib.rs

+	/// Run the given closure `run` and grant it read access to the raw memory.
+	fn with_access<R>(&self, run: impl FnOnce(&[u8]) -> R) -> R;
+	/// Grow the memory by `additional` pages.
+	fn grow(&mut self, additional: u32) -> Result<(), ()>;


Just curious (as a general practice). Why not just return bool?

If you use Result it's immediately obvious that the function can error out, and that Result::Err is an error. And you'll get a warning if the Result is not handled (since it's marked with #[must_use]). With bool there's no such convention.

And of course with Result you can use e.g. map_err and ?.

Good question, kind of taste? I mean it is a failure and a simple bool feels too much like C, but semantically it doesn't make any difference.

client/allocator/src/freeing_bump.rs

davxy · 2023-02-21T18:01:09Z

client/allocator/src/freeing_bump.rs

 	#[test]
 	fn should_allocate_properly() {
 		// given
-		let mut mem = [0u8; PAGE_SIZE as usize];
+		let mut mem = MemoryInstance::with_pages(1);
 		let mut heap = FreeingBumpHeapAllocator::new(0);


Dumb question (I just drop it here even if not strictly related to this code snip).

What is the motivation behind the design decision of keeping the managed memory buffer (in this case MemoryInstance) separated from the heap structure manager (i.e. in this case FreeingBumpHeapAllocator).

Why not add a MemoryInstance field to the heap structure?

As far as I understood the heap structure maintains a bunch of information strictly bounded to the buffer it operates on... perhaps the motivation is to pass it between the host and the runtime without and prevent the whole buffer copy? Just guessing 😃

Yeah should probably now be achievable, before with just operating on the bytes it would have required a trait (which we now got). Maybe some possible future refactor, but is also not such a huge problem.

In case of wasmtime the actual memory is owned by the executor, so to access it you also need a mutable handle to the context, but you're also storing the FreeingBumpHeapAllocator inside the context, and you can't have two mutable references to the same thing. That's why Basti has to store the allocator inside of an Option and temporarily take() it in client/executor/wasmtime/src/host.rs.

Technically you could add a field inside of the FreeingBumpHeapAllocator which would store a handle to the memory, but it still wouldn't own the memory (because it's owned by wasmtime and the Memory it exposes is just essentially an usize index into wasmtime's internal store) and you still would need wasmtime's context to access it. So there isn't really much point in doing that.

client/executor/benches/bench.rs

davxy · 2023-02-21T18:29:08Z

client/executor/common/src/runtime_blob/runtime_blob.rs

+			let (min, max) = match heap_alloc_strategy {
+				HeapAllocStrategy::Dynamic { maximum_pages } => {
+					// Ensure `initial <= maximum_pages`
+					(maximum_pages.map(|m| m.min(initial)).unwrap_or(initial), maximum_pages)


Isn't initial the value that drives the min?
With this code if maximum_pages is Some and maximum_pages < initial then we set min to maximum_pages value. And also max is set to maximum_pages resulting in (min = max) and (max < initial)

We don't want initial to be greater then max. See the comment above.

Indeed. But what I was saying is that if the program requires an initial greater than maximum_pages we end up giving to the program less memory than how much it requires. I may have missed something anyway 🤔 And maybe that is intentional?

Yes that is intentional. If something requires more, then it will probably fail at execution.

client/executor/src/native_executor.rs

client/executor/wasmi/src/lib.rs

bkchr · 2023-02-23T11:21:01Z

DQ: Having different memory model for benchmarking (dynamic) and for runtime execution (static) may lead to wrongly estimated cost of memory. For example in case of allocation failure the runtime code may fallback to more expensive algorithm (imagine doing something in place instead of using extra buffer). Not sure if this is applicable here.

When the allocation fails, we stop the execution. The runtime doesn't see that the allocation failed.

bkchr · 2023-02-24T11:42:52Z

bot merge

* Companion for Substrate#12520 paritytech/substrate#12520 * Fix * update lockfile for {"substrate"} --------- Co-authored-by: parity-processbot <>

* As always, start with something :P * Add support for max_heap_pages * Add support for wasmtime * Make it compile * Fix compilation * Copy wrongly merged code * Fix compilation * Some fixes * Fix * Get stuff working * More work * More fixes * ... * More * FIXEs * Switch wasmi to use `RuntimeBlob` like wasmtime * Removed unused stuff * Cleanup * More cleanups * Introduce `CallContext` * Fixes * More fixes * Add builder for creating the `WasmExecutor` * Adds some docs * FMT * First round of feedback. * Review feedback round 2 * More fixes * Fix try-runtime * Update client/executor/wasmtime/src/instance_wrapper.rs Co-authored-by: Koute <[email protected]> * Update client/executor/common/src/wasm_runtime.rs Co-authored-by: Koute <[email protected]> * Update client/executor/common/src/runtime_blob/runtime_blob.rs Co-authored-by: Koute <[email protected]> * Update client/executor/common/src/wasm_runtime.rs Co-authored-by: Koute <[email protected]> * Update client/allocator/src/freeing_bump.rs Co-authored-by: Koute <[email protected]> * Update client/allocator/src/freeing_bump.rs Co-authored-by: Koute <[email protected]> * Feedback round 3 * FMT * Review comments --------- Co-authored-by: Koute <[email protected]>

bkchr added 30 commits August 6, 2021 22:01

As always, start with something :P

4dbc715

Add support for max_heap_pages

639dbaf

Add support for wasmtime

b251bda

Merge branch 'bkchr-grow-memory' of github.com:paritytech/substrate i…

b2e5d1e

…nto bkchr-grow-memory

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

b07dc47

Merge branch 'bkchr-grow-memory' of github.com:paritytech/substrate i…

79303bb

…nto bkchr-grow-memory

Make it compile

ecf5031

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

e99a9dd

Fix compilation

0840615

Copy wrongly merged code

945b08b

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

9380a48

Fix compilation

a2d9290

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

11348ae

Some fixes

8e746fb

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

0032da0

Fix

9bbc1ab

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

545f225

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

ac1558b

Get stuff working

0536fc4

More work

128b3ab

More fixes

984b74d

...

d990d20

More

a9abd56

FIXEs

3da8cdc

Switch wasmi to use RuntimeBlob like wasmtime

b18f871

Removed unused stuff

30c6ac8

Cleanup

113353a

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

a8732ff

More cleanups

5d54905

Introduce CallContext

8b2ec2b

bkchr and others added 5 commits February 17, 2023 15:24

Update client/executor/common/src/wasm_runtime.rs

1bc71ff

Co-authored-by: Koute <[email protected]>

Update client/allocator/src/freeing_bump.rs

0c2fbe5

Co-authored-by: Koute <[email protected]>

Update client/allocator/src/freeing_bump.rs

2c234a5

Co-authored-by: Koute <[email protected]>

Feedback round 3

4378b30

FMT

c7c3117

koute approved these changes Feb 20, 2023

View reviewed changes

michalkucharczyk reviewed Feb 21, 2023

View reviewed changes

client/executor/wasmi/src/lib.rs Show resolved Hide resolved

michalkucharczyk reviewed Feb 21, 2023

View reviewed changes

davxy approved these changes Feb 21, 2023

View reviewed changes

michalkucharczyk approved these changes Feb 21, 2023

View reviewed changes

skunert approved these changes Feb 23, 2023

View reviewed changes

bkchr added 2 commits February 23, 2023 12:36

Review comments

1f41507

Merge remote-tracking branch 'origin/master' into bkchr-grow-memory

ac27b2e

paritytech-processbot bot merged commit 1837f42 into master Feb 24, 2023

paritytech-processbot bot deleted the bkchr-grow-memory branch February 24, 2023 11:43

nazar-pc mentioned this pull request Feb 28, 2023

The paging file is too small for this operation to complete #13487

Closed

2 tasks

jasl mentioned this pull request Mar 21, 2023

Upgrade to Polkadot v0.9.39 Phala-Network/phala-blockchain#1203

Merged

1xstj mentioned this pull request Mar 31, 2023

bump to polkadot-v0.9.39 tangle-network/tangle#156

Merged

5 tasks

kiltbot mentioned this pull request Apr 3, 2023

[AUTOMATIC] Update Polkadot dependencies from 0.9.38 to 0.9.39 KILTprotocol/kilt-node#495

Closed

librelois mentioned this pull request Apr 6, 2023

Update substrate/polkadot/cumulus from v0.9.38 to v0.9.40 moonbeam-foundation/moonbeam#2200

Closed

kacperzuk-neti mentioned this pull request Jun 23, 2023

Polkadot v0.9.43 liberland/liberland_substrate#295

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasm-executor: Support growing the memory #12520

wasm-executor: Support growing the memory #12520

bkchr commented Oct 18, 2022 •

edited

Loading

koute left a comment

michalkucharczyk Feb 21, 2023

michalkucharczyk commented Feb 21, 2023

davxy Feb 21, 2023

koute Feb 23, 2023

bkchr Feb 23, 2023

davxy Feb 21, 2023

bkchr Feb 23, 2023

koute Feb 23, 2023

davxy Feb 21, 2023

michalkucharczyk Feb 21, 2023 •

edited

Loading

davxy Feb 21, 2023 •

edited

Loading

bkchr Feb 23, 2023

bkchr commented Feb 23, 2023

bkchr commented Feb 24, 2023

wasm-executor: Support growing the memory #12520

wasm-executor: Support growing the memory #12520

Conversation

bkchr commented Oct 18, 2022 • edited Loading

Why supporting for different maximum heap pages, based on the context?

Enabling dynamic memory

koute left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkucharczyk commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkucharczyk Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

davxy Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented Feb 23, 2023

bkchr commented Feb 24, 2023

bkchr commented Oct 18, 2022 •

edited

Loading

michalkucharczyk Feb 21, 2023 •

edited

Loading

davxy Feb 21, 2023 •

edited

Loading