-
Notifications
You must be signed in to change notification settings - Fork 443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Bump Allocator #831
Add Bump Allocator #831
Conversation
1kB savings are already a lot since we are talking about a bottleneck issue here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please implement the suggestions and try again and see whether it resulted in another win with respect to Wasm file size.
This will reduce our use of dependencies which will hopefully reduce our final Wasm binary size. Also, apparently spinlocks aren't actually all that efficient. See: https://matklad.github.io/2020/01/02/spinlocks-considered-harmful.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes because the contains a bug if I am not mistaken.
Panicking in the global allocator is considered undefined behaviour.
@Robbepop I've updated the numbers in the table. They stayed the same for the most part, with the exception of two places where the contract size went up by |
Haven't looked at the code yet, but it would be great to have fuzz tests for this allocator (could be in a follow-up). We have a number of fuzz tests for data structures, etc.. Those are run after each merge to |
Yeah, that would be a good idea! |
@@ -65,3 +65,4 @@ std = [ | |||
# Enable contract debug messages via `debug_print!` and `debug_println!`. | |||
ink-debug = [] | |||
ink-experimental-engine = ["ink_engine"] | |||
wee-alloc = ["ink_allocator/wee-alloc"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the plan for wee-alloc
? I mean, why even keep it in here? If there is some valuable trade-off between both of them we should add some info on it to the docs: https://paritytech.github.io/ink-docs/datastructures/dynamic-allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tradeoff here is .wasm
size vs. efficient use of memory.wee_alloc
is able to re-use freed memory, while the bump
allocator is not.
I agree though, if we do keep both it would be valuable to document the trade-offs. I've opened use-ink/ink-docs#24.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I like the simplicity :-).
Could you create a follow-up issue for adding fuzz tests please?
Sure, I opened #860. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some suggestions for simplification of the ifdef expressions.
This PR adds a simple bump allocator based off this great tutorial: https://os.phil-opp.com/allocator-designs/#bump-allocator.
The goal here is to see whether or not a simpler allocator design will be able to
significantly reduce the space footprint of contracts which use it. See #827 for more
details.
Some things to note:
While this PR compiles, contracts compiled with the bump allocator panic on-chainFixed, thanks Alex!(
ContractTrapped
!). I need to look into thisI haven't been able to initalize the heap in a non-sketchy way - it's been a littleThis isn't relevant anymorehard with the requirements from
#[global_allocator]
Early Results
I've built our example contracts with both the Bump allocator and the Wee allocator.
contract-terminate
contract-transfer
dns
erc1155
erc20
erc721
flipper
incrementer
multisig_plain
rand-extension
trait-erc20
trait-flipper
accumulator
adder
subber
delegator
As you can see, the savings are generally around 1K, which really isn't a whole lot given
how much better
wee_alloc
is over the bump allocator.