Skip to content
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

Merged
merged 41 commits into from
Jul 20, 2021
Merged

Add Bump Allocator #831

merged 41 commits into from
Jul 20, 2021

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Jun 28, 2021

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-chain
    (ContractTrapped!). I need to look into this
    Fixed, thanks Alex!
  • I haven't been able to initalize the heap in a non-sketchy way - it's been a little
    hard with the requirements from #[global_allocator]
    This isn't relevant anymore

Early Results

I've built our example contracts with both the Bump allocator and the Wee allocator.

Contract Original (Bump Alloc) Original (Wee Alloc) Optimized (Bump Alloc) Optimized (Wee Alloc)
contract-terminate 18.7K 18.7K 2.1K 2.1K
contract-transfer 26.3K 27.4K 9.3K 10.3K
dns 44.2K 45.5K 25.6K 26.5K
erc1155 65.2K 66.8K 45.3K 46.8K
erc20 51.8K 53.2K 30.5K 31.8K
erc721 68.3K 69.8K 41.1K 42.5K
flipper 19.0K 19.0K 2.4K 2.4K
incrementer 19.0K 19.0K 2.4K 2.4K
multisig_plain 86.2K 87.3K 53.6K 54.7K
rand-extension 22.2K 22.2K 5.2K 5.2K
trait-erc20 52.3K 53.7K 31.0K 32.2K
trait-flipper 19.0K 19.0K 2.4K 2.4K
accumulator 25.7K 25.7K 8.5K 8.5K
adder 30.3K 30.3K 12.8K 12.8K
subber 30.3K 30.3K 12.8K 12.8K
delegator 28.3K 28.3K 10.8K 10.8K

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.

@HCastano HCastano added A-ink_storage [ink_storage] Work Item E-in-progress A task that is already being worked on. labels Jun 28, 2021
@Robbepop
Copy link
Collaborator

Robbepop commented Jun 29, 2021

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.

1kB savings are already a lot since we are talking about a bottleneck issue here.
Execution times of Wasm smart contracts are less important at the moment than their final Wasm file sizes.
Many of the examples show zero difference since ink! is very good at optimizing away the allocator entirely which is good and expected.
Maybe we can do even better than this, I need to view the code closely.
Also I think the bump allocator should also be able to perform better since it needs less management due to allocated memory not being able to deallocate.

Copy link
Collaborator

@Robbepop Robbepop left a 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.

crates/allocator/Cargo.toml Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
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
Copy link
Contributor

@athei athei left a 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.

crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Show resolved Hide resolved
crates/allocator/src/bump.rs Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/Cargo.toml Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/lib.rs Outdated Show resolved Hide resolved
@HCastano
Copy link
Contributor Author

HCastano commented Jun 30, 2021

@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 0.2K and 0.3K.

crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
@HCastano HCastano marked this pull request as ready for review July 13, 2021 15:29
@HCastano HCastano requested review from athei and Robbepop July 13, 2021 15:29
@HCastano HCastano removed the E-in-progress A task that is already being worked on. label Jul 13, 2021
@cmichi
Copy link
Collaborator

cmichi commented Jul 15, 2021

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 master for some time.

@HCastano
Copy link
Contributor Author

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 master for some time.

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"]
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

@cmichi cmichi left a 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?

@HCastano
Copy link
Contributor Author

LGTM! I like the simplicity :-).

Could you create a follow-up issue for adding fuzz tests please?

Sure, I opened #860.

Copy link
Contributor

@athei athei left a 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.

crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
crates/allocator/src/bump.rs Outdated Show resolved Hide resolved
@HCastano HCastano dismissed Robbepop’s stale review July 20, 2021 14:36

Two Approvals Already Given

@HCastano HCastano merged commit 5fd8f87 into master Jul 20, 2021
@HCastano HCastano deleted the hc-bump-allocator branch July 20, 2021 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_storage [ink_storage] Work Item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants