Better allocator for wasm #1460

cmichi · 2019-01-16T21:11:35Z

Hi there,

this PR addresses #300.

Some notes:

The prior linear allocator didn't do any error handling, this allocator does. I went for panic!, since the allocation returns u32. Is panic! appropriate?
Tracking the heap size is in there because it came handy for writing the tests. Should we remove it for production because of performance or do you think the trace! could still be helpful?
The size of allocated bytes per pointer needs to be stored, so that deallocate can properly free allocated blocks. This size is currently stored in a HashMap and there might be a more performant way.
The alignment of u8 on unknown-unknown-webassembly is indeed 1; this was validated by @pepyakin .
Once a block is allocated it fills up the block space ‒ no matter how full it is. I chose a block size of 8192 Bytes because it seemed like a reasonable trade-off between wasting space on the heap and still keeping the whole thing performant. I tried bigger block sizes as well, but the stack then quickly reaches it's limit and a number of tests fail.

I haven't done a proper benchmark yet, but I examined the average heap size when running a --dev chain. The difference looks very good (of course though, since the linear allocator never freed anything up). I haven't looked at the difference in execution time yet.

The purose of this big block is for the tests to test a failure when the block is too big though. The improved buddy allocation strategy results in this block fitting on the heap now. Hence the increase.

parity-cla-bot · 2019-01-16T21:11:39Z

It looks like @cmichi hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

cmichi · 2019-01-16T21:12:43Z

[clabot:check]

parity-cla-bot · 2019-01-16T21:12:45Z

It looks like @cmichi signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

core/executor/src/heap.rs

pepyakin

Gave a brief review, without checking the logic of the allocator. Will do a proper review tomorrow

README.adoc

core/executor/src/heap.rs

core/executor/src/wasm_executor.rs

pepyakin · 2019-01-16T22:45:14Z

core/sr-io/without_std.rs

-			let ret = slice::from_raw_parts(ptr, length as usize).to_vec();
-			ext_free(ptr);
-			Some(ret)
+			Some(<Vec<u8>>::from_raw_parts(ptr, length as usize, length as usize))


As was discussed in the issue, this actually violates formal preconditions required by Vec::from_raw_parts (i.e. the pointer is allocated not with Vec routines). Given that we agree with it, I still would like to have a comment here mentioning this fact.

And there should be another case nearby (below).

I'm still against using Vec::from_raw_parts because of all the invariants. If we can prove the invariants, I'm happy.

@gavofyork What is your opinion on this?

It may be better to return some other new type as well.

As I stated in the other thread I'm against this UB dogma. I demonstrated that this code is safe. If you think otherwise please bring the argument forward, otherwise this code goes in.

Yes, I'd agree that it would work for us and I don't have anymore to add to our discussion here! But I'd still be in favor of a comment here. I hope it will save us a headache.

Also, @cmichi there is another case where we apply the same pattern: child_storage, a function below. I'd like them to be synchronized.

To prevent misunderstandings of people executing both.

README.adoc

core/executor/src/heap.rs

bkchr · 2019-01-17T08:15:15Z

core/sr-io/without_std.rs

-			let ret = slice::from_raw_parts(ptr, length as usize).to_vec();
-			ext_free(ptr);
-			Some(ret)
+			Some(<Vec<u8>>::from_raw_parts(ptr, length as usize, length as usize))


I'm still against using Vec::from_raw_parts because of all the invariants. If we can prove the invariants, I'm happy.

pepyakin · 2019-01-17T14:24:25Z

Even though CI succeeds, it looks like there are a bunch of tests failing because of overflows, e.g.

---- wasm_executor::tests::twox_128_should_work stdout ----
thread 'wasm_executor::tests::twox_128_should_work' panicked at 'attempt to multiply with overflow', core/executor/src/wasm_executor.rs:61:19

CI is green because we are running tests in release but without debug assertions...

core/executor/src/wasm_executor.rs

core/executor/src/heap.rs

This reverts commit 292c177.

core/executor/src/wasm_executor.rs

core/executor/src/heap.rs

pepyakin · 2019-01-18T13:49:30Z

core/executor/src/heap.rs

+	///   (in bytes) for allocating memory.
+	///
+	pub fn new(mut ptr_offset: u32, heap_size: u32) -> Self {
+		assert!(BLOCK_SIZE % ALIGNMENT == 0, "Block size is no multiple of alignment!");


Nit: I have a slight preference for moving it into tests, since if you think about it it doesn't have anything to do with a constructor.

You can do it at compile time too with this method, because mem::transmute checks that type sizes are the same :)

const BLOCK_SIZE: usize = 1024; const ALIGNMENT: usize = 8; // try changing to 9 fn _assert_block_size_aligned() { ::std::mem::transmute::<[u8; BLOCK_SIZE % ALIGNMENT], [u8; 0]>; } fn main() { }

Hmm, I added it to the constructor with the intention of asserting that a Heap can only ever be created if the alignment is correct, no matter if tests have been run or not. Do you think assertions don't have a place outside of tests in general?

I'd consider not tested binary build broken, but yeah : ) I actually like @rphmeier 's approach since it will prevent this from compiling, and doesn't require a separate crate like static_assert for doing that!

pepyakin

Well done! I have only one small nit

This reverts commit c3bee59.

* Revert "Better allocator for wasm (#1460)" This reverts commit c3bee59. * Update wasm files

* Add Arch Linux installation instructions * Enable tracing heap size * Extract heap * Replace linear allocator with buddy allocator * Fix test The purose of this big block is for the tests to test a failure when the block is too big though. The improved buddy allocation strategy results in this block fitting on the heap now. Hence the increase. * Get rid of memcpy in to_vec() * fixup: Style and comments * fixup: Split Linux instructions by distribution To prevent misunderstandings of people executing both. * fixup: Remove unnecessary types and code * fixup: Make Pointers start from 1, remove some panics, code improvements * fixup: Return 0 on errors * fixup: Move loop to separate function * fixup: Use FnvHashMap instead of HashMap * fixup: Fix error handling * fixup: Use current_size() instead of used_size() * fixup: Fix and document allocation offset * fixup: Remove unnecessary multiplication * fixup: Fix comments * fixup: Remove Arch installation instructions * Revert "Fix test" This reverts commit 292c177. * fixup: Remove unused code, improve import * fixup: Proper alignment * fixup: Do not use internal constant in public description * fixup: Add comment regarding invariants * fixup: Move assertion to compile-time check

* Better allocator for wasm (#1460) * Add Arch Linux installation instructions * Enable tracing heap size * Extract heap * Replace linear allocator with buddy allocator * Fix test The purose of this big block is for the tests to test a failure when the block is too big though. The improved buddy allocation strategy results in this block fitting on the heap now. Hence the increase. * Get rid of memcpy in to_vec() * fixup: Style and comments * fixup: Split Linux instructions by distribution To prevent misunderstandings of people executing both. * fixup: Remove unnecessary types and code * fixup: Make Pointers start from 1, remove some panics, code improvements * fixup: Return 0 on errors * fixup: Move loop to separate function * fixup: Use FnvHashMap instead of HashMap * fixup: Fix error handling * fixup: Use current_size() instead of used_size() * fixup: Fix and document allocation offset * fixup: Remove unnecessary multiplication * fixup: Fix comments * fixup: Remove Arch installation instructions * Revert "Fix test" This reverts commit 292c177. * fixup: Remove unused code, improve import * fixup: Proper alignment * fixup: Do not use internal constant in public description * fixup: Add comment regarding invariants * fixup: Move assertion to compile-time check * Fix bug in necessary tree level calculation The tree levels necessary to house a number of nodes was calculated incorrectly. * Improve naming

* Add Arch Linux installation instructions * Enable tracing heap size * Extract heap * Replace linear allocator with buddy allocator * Fix test The purose of this big block is for the tests to test a failure when the block is too big though. The improved buddy allocation strategy results in this block fitting on the heap now. Hence the increase. * Get rid of memcpy in to_vec() * fixup: Style and comments * fixup: Split Linux instructions by distribution To prevent misunderstandings of people executing both. * fixup: Remove unnecessary types and code * fixup: Make Pointers start from 1, remove some panics, code improvements * fixup: Return 0 on errors * fixup: Move loop to separate function * fixup: Use FnvHashMap instead of HashMap * fixup: Fix error handling * fixup: Use current_size() instead of used_size() * fixup: Fix and document allocation offset * fixup: Remove unnecessary multiplication * fixup: Fix comments * fixup: Remove Arch installation instructions * Revert "Fix test" This reverts commit 292c177. * fixup: Remove unused code, improve import * fixup: Proper alignment * fixup: Do not use internal constant in public description * fixup: Add comment regarding invariants * fixup: Move assertion to compile-time check

* Revert "Better allocator for wasm (paritytech#1460)" This reverts commit c3bee59. * Update wasm files

* Better allocator for wasm (paritytech#1460) * Add Arch Linux installation instructions * Enable tracing heap size * Extract heap * Replace linear allocator with buddy allocator * Fix test The purose of this big block is for the tests to test a failure when the block is too big though. The improved buddy allocation strategy results in this block fitting on the heap now. Hence the increase. * Get rid of memcpy in to_vec() * fixup: Style and comments * fixup: Split Linux instructions by distribution To prevent misunderstandings of people executing both. * fixup: Remove unnecessary types and code * fixup: Make Pointers start from 1, remove some panics, code improvements * fixup: Return 0 on errors * fixup: Move loop to separate function * fixup: Use FnvHashMap instead of HashMap * fixup: Fix error handling * fixup: Use current_size() instead of used_size() * fixup: Fix and document allocation offset * fixup: Remove unnecessary multiplication * fixup: Fix comments * fixup: Remove Arch installation instructions * Revert "Fix test" This reverts commit 292c177. * fixup: Remove unused code, improve import * fixup: Proper alignment * fixup: Do not use internal constant in public description * fixup: Add comment regarding invariants * fixup: Move assertion to compile-time check * Fix bug in necessary tree level calculation The tree levels necessary to house a number of nodes was calculated incorrectly. * Improve naming

Update Rust toolchain to latest nightly

cmichi and others added 6 commits January 16, 2019 21:42

Add Arch Linux installation instructions

2741554

Enable tracing heap size

e95140a

Extract heap

d2a7935

Replace linear allocator with buddy allocator

aff276a

Fix test

292c177

The purose of this big block is for the tests to test a failure when the block is too big though. The improved buddy allocation strategy results in this block fitting on the heap now. Hence the increase.

Get rid of memcpy in to_vec()

1d71e15

cmichi requested review from gavofyork and pepyakin January 16, 2019 21:20

cmichi added the A0-please_review Pull request needs code review. label Jan 16, 2019

rphmeier reviewed Jan 16, 2019

View reviewed changes

core/executor/src/heap.rs Show resolved Hide resolved

rphmeier reviewed Jan 16, 2019

View reviewed changes

core/executor/src/heap.rs Outdated Show resolved Hide resolved

pepyakin reviewed Jan 16, 2019

View reviewed changes

cmichi added 2 commits January 17, 2019 09:10

fixup: Style and comments

b65f0e4

fixup: Split Linux instructions by distribution

02a3d1e

To prevent misunderstandings of people executing both.

bkchr reviewed Jan 17, 2019

View reviewed changes

cmichi added 8 commits January 17, 2019 10:19

fixup: Remove unnecessary types and code

dfa62a8

fixup: Make Pointers start from 1, remove some panics, code improvements

41e4ab7

fixup: Return 0 on errors

53bf828

fixup: Move loop to separate function

b189736

fixup: Use FnvHashMap instead of HashMap

18a1efc

fixup: Fix error handling

137b5bd

fixup: Use current_size() instead of used_size()

741e972

fixup: Fix and document allocation offset

93dc2e9

pepyakin reviewed Jan 17, 2019

View reviewed changes

core/executor/src/wasm_executor.rs Outdated Show resolved Hide resolved

fixup: Remove unnecessary multiplication

d4676b0

pepyakin reviewed Jan 17, 2019

View reviewed changes

core/executor/src/heap.rs Outdated Show resolved Hide resolved

cmichi added 2 commits January 17, 2019 15:50

fixup: Fix comments

6a99426

fixup: Remove Arch installation instructions

6725a49

bkchr reviewed Jan 17, 2019

View reviewed changes

core/executor/src/heap.rs Outdated Show resolved Hide resolved

Revert "Fix test"

6438db3

This reverts commit 292c177.

pepyakin reviewed Jan 17, 2019

View reviewed changes

core/executor/src/wasm_executor.rs Outdated Show resolved Hide resolved

fixup: Remove unused code, improve import

ecd9731

pepyakin reviewed Jan 17, 2019

View reviewed changes

core/executor/src/heap.rs Show resolved Hide resolved

cmichi added 2 commits January 17, 2019 19:18

fixup: Proper alignment

ca065b1

fixup: Do not use internal constant in public description

58ee588

gavofyork approved these changes Jan 17, 2019

View reviewed changes

pepyakin reviewed Jan 18, 2019

View reviewed changes

pepyakin approved these changes Jan 18, 2019

View reviewed changes

pepyakin added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jan 18, 2019

cmichi added 2 commits January 18, 2019 15:28

fixup: Add comment regarding invariants

247d37b

fixup: Move assertion to compile-time check

8e4deda

pepyakin merged commit c3bee59 into master Jan 18, 2019

pepyakin deleted the cmichi-better-allocator-for-wasm branch January 18, 2019 16:29

pepyakin mentioned this pull request Jan 18, 2019

Better allocator for Wasm #300

Closed

xlc mentioned this pull request Jan 21, 2019

Consensus error between wasm and native runtime execution in Metadata_metadata #1495

Closed

bkchr added a commit that referenced this pull request Jan 21, 2019

Revert "Better allocator for wasm (#1460)"

2188c78

This reverts commit c3bee59.

bkchr added a commit that referenced this pull request Jan 21, 2019

Revert new wasm allocator (#1502)

4d0eea0

* Revert "Better allocator for wasm (#1460)" This reverts commit c3bee59. * Update wasm files

MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019

Revert new wasm allocator (paritytech#1502)

f7f61f5

* Revert "Better allocator for wasm (paritytech#1460)" This reverts commit c3bee59. * Update wasm files

cmichi mentioned this pull request Sep 28, 2022

Member Request polkadot-fellows/seeding#19

Closed

liuchengxu pushed a commit to liuchengxu/substrate that referenced this pull request May 31, 2023

Merge pull request paritytech#1460 from subspace/update-rust

9b83b80

Update Rust toolchain to latest nightly

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better allocator for wasm #1460

Better allocator for wasm #1460

cmichi commented Jan 16, 2019 •

edited

Loading

parity-cla-bot commented Jan 16, 2019

cmichi commented Jan 16, 2019

parity-cla-bot commented Jan 16, 2019

pepyakin left a comment

pepyakin Jan 16, 2019 •

edited

Loading

bkchr Jan 17, 2019

cmichi Jan 17, 2019

rphmeier Jan 17, 2019

gavofyork Jan 17, 2019

pepyakin Jan 18, 2019

bkchr Jan 17, 2019

pepyakin commented Jan 17, 2019 •

edited

Loading

pepyakin Jan 18, 2019

rphmeier Jan 18, 2019 •

edited

Loading

cmichi Jan 18, 2019

pepyakin Jan 18, 2019

pepyakin left a comment

Better allocator for wasm #1460

Better allocator for wasm #1460

Conversation

cmichi commented Jan 16, 2019 • edited Loading

parity-cla-bot commented Jan 16, 2019

cmichi commented Jan 16, 2019

parity-cla-bot commented Jan 16, 2019

pepyakin left a comment

Choose a reason for hiding this comment

pepyakin Jan 16, 2019 • edited Loading

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

pepyakin commented Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

rphmeier Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin left a comment

Choose a reason for hiding this comment

cmichi commented Jan 16, 2019 •

edited

Loading

pepyakin Jan 16, 2019 •

edited

Loading

pepyakin commented Jan 17, 2019 •

edited

Loading

rphmeier Jan 18, 2019 •

edited

Loading