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

Remove dynamic storage allocator #1148

Merged
merged 7 commits into from
Feb 28, 2022
Merged

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Feb 26, 2022

Yet another follow up related to #1111.

In this case we can safely remove the dynamic storage allocator and related code since we
removed all collections which can allocate storage dynamically.

Will need follow up in the ink! documentation portal and workshop.

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2022

Codecov Report

Merging #1148 (bb5a83d) into master (3527918) will increase coverage by 3.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   75.40%   78.70%   +3.29%     
==========================================
  Files         254      243      -11     
  Lines        9446     9242     -204     
==========================================
+ Hits         7123     7274     +151     
+ Misses       2323     1968     -355     
Impacted Files Coverage Δ
crates/lang/codegen/src/generator/dispatch.rs 94.52% <ø> (-0.05%) ⬇️
crates/lang/ir/src/ir/contract.rs 100.00% <ø> (+100.00%) ⬆️
crates/lang/macro/src/lib.rs 100.00% <ø> (ø)
crates/lang/src/codegen/dispatch/execution.rs 0.00% <ø> (ø)
crates/lang/ir/src/ir/config.rs 95.52% <100.00%> (+25.90%) ⬆️
crates/primitives/src/key_ptr.rs 75.00% <0.00%> (-25.00%) ⬇️
crates/storage/src/collections/bitstash/mod.rs 93.75% <0.00%> (-4.17%) ⬇️
crates/storage/src/lazy/mod.rs 83.87% <0.00%> (-1.43%) ⬇️
crates/lang/ir/src/ir/attrs.rs 82.27% <0.00%> (+3.60%) ⬆️
crates/lang/ir/src/ir/item_impl/callable.rs 91.91% <0.00%> (+4.04%) ⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3527918...bb5a83d. Read the comment docs.

@Robbepop
Copy link
Collaborator

Yet another follow up related to #1111.

In this case we can safely remove the dynamic storage allocator and related code since we removed all collections which can allocate storage dynamically.

Will need follow up in the ink! documentation portal and workshop.

Glad to see this. To be honest, the dynamic storage allocator should not be revived later on since it was more or less a failed experiment. If we want to support use cases that are impossible otherwise we should rather add proper support in contracts-pallet via the often discussed bulk memory removal or tree-like storage management.

@paritytech-cicd-pr
Copy link

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

⚠️ The ink! master is ahead of your branch, this might skew the comparison data below.

These are the results when building the examples/* contracts from this branch with cargo-contract 0.17.0-638ad57 and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator -0.01 K 1.28 K
adder 2.23 K
contract-terminate 1.23 K 214_418
contract-transfer 8.37 K 14_418
delegator -0.02 K -3 6.24 K 46_504
dns -0.02 K 9.62 K 43_254
erc1155 -0.04 K 27.49 K 86_508
erc20 -0.03 K 9.09 K 43_254
erc721 -0.03 K 14.37 K 115_344
flipper -0.02 K 1.55 K 14_418
incrementer -0.01 K 1.49 K 14_418
multisig -0.02 K 26.28 K 94_127
proxy 2.98 K 29_508
rand-extension -0.03 K 4.38 K 14_418
subber 2.24 K
trait-erc20 -0.03 K 9.36 K 43_254
trait-flipper -0.02 K 1.33 K 14_418
trait-incrementer -0.03 K 1.42 K 28_836

Link to the run | Last update: Mon Feb 28 09:23:15 CET 2022

@cmichi cmichi merged commit c94d39c into master Feb 28, 2022
@cmichi cmichi deleted the hc-remove-dynamic-storage-allocator branch February 28, 2022 08:26
@cmichi
Copy link
Collaborator

cmichi commented Feb 28, 2022

Will need follow up in the ink! documentation portal and workshop.

@HCastano Could you do that please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants