Hide `collection` and `lazy` modules #1111

HCastano · 2022-01-25T05:54:39Z

The goal of this PR is to begin the process of removing all data structures other than
Mapping from the public ink! API. We're starting by just hiding the collections and
lazy modules from users. Eventually we'll want to remove the code entirely, but I
wanted to make the minimal amount of changes required to get this compiling.

Rational

You may be asking: why remove all our great collections? Well, if you've noticed a theme
in ink! over the last few months its been that of reducting complexity in order to
reduce the size of contracts. This takes another step towards that.

Until we can provide full-featured collections which don't bloat contract sizes the only
reasonable step forward to (somewhat strongly) encourage developers to use Mapping as
their main "data type".

Eager Loading

One consequence of this PR is that because there is no more Lazy type, every single
storage item from your contract will be loaded from storage eagerly. To give a more
concrete example, consider the following contract storage struct:

#[ink(storage)]
pub struct Contract {
    ids: u32,
    accounts: ink_prelude::vec::Vec<AccountId>,
}

In this case every time we execute the contract we'd load up the entire Vec from
storage (even if we didn't use the Vec during the execution). For large Vec this
would be inefficient, but it's something you as a developer will need to be mindful of
going forward.

Compatibility

Unfortunately, this will be a breaking change for any contract using collections
other than Mapping. If your contracts were mainly just using HashMap/BTreeMap the
migration should be pretty straightforward - although during the migration you will
notice that the API for Mapping is more rudamentary (by design) than the latter two
data structures.

Follow-Ups

Depending on the reception to this, I'll do the actual code pruning in some follow up
PRs.
There are some TODOs with XYZ as the issue numbers. Once I make those issues I'll
update the PR to fill them in.
Since we don't have collections which need dynamic allocations anymore I believe that
we can get rid of the dynamic storage allocator code entirely. Someone correct me if
I'm wrong here.
There's definitely documentation internal to ink! and external (such as on our docs
portal) that will need to be updated
More ergonomic way to use ink_lang::codegen::initialize_contract() for initializing
Mapping's

Related issue: #996

These use types from `collections` which are not available publicly anymore.

It doesn't looks like there's a way to keep these compiling if we're hiding the collection module, so the next best thing to do is to remove them completely.

athei

I agree that we should be able to hide/remove the storage alocator. I think you need to make the CI happy in order to get our change break down (gas/size), right? Or is this still broken from the storage deposit fallout? @cmichi

crates/lang/macro/src/lib.rs

crates/storage/src/collections/binary_heap/mod.rs

athei · 2022-01-28T16:28:23Z

crates/storage/src/lib.rs

@@ -62,9 +68,13 @@ mod test_utils;

 #[doc(inline)]
 pub use self::{
-    alloc::Box,
+    lazy::Mapping,
+    memory::Memory,


Do we really need this? It feels like an anti pattern to make use of this. Data should be passed as an argument where possible IMHO. Also, I think supporting this in the future will be annoying.

Tbh, I'm not sure. I haven't seen any ink! contracts using Memory, but I know it's used in Solidity contracts. Maybe @cmichi or @ascjones can chime in here?

No strong opinion here. I have a bad feeling about not having any example using this though. In consequence we don't have any proper on-chain ink-waterfall test for it either.

Data should be passed as an argument where possible IMHO.

Agree, but I don't know enough about why it was introduced in Solidity in the first place.

Does @ascjones know more?

HCastano · 2022-01-28T23:48:35Z

I think you need to make the CI happy in order to get our change break down (gas/size), right?

There was an issue with GitHub access tokens that got fixed a couple days ago. I'm just gonna re-run the CI and it should post it

paritytech-cicd-pr · 2022-01-28T23:58:03Z

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

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

	Δ Optimized Size	Δ Used Gas	Total Optimized Size	Total Used Gas
`accumulator`			1.44 K
`adder`			2.94 K
`contract-terminate`			1.31 K	200_778
`contract-transfer`			7.98 K	1_399
`delegator`	-2.43 K	+786	7.63 K	10_296
`dns`	-0.58 K	+538	10.20 K	15_153
`erc1155`			27.50 K	47_811
`erc20`	-0.42 K	+257	9.80 K	10_014
`erc721`			13.70 K	34_702
`flipper`			1.76 K	347
`incrementer`			1.64 K	340
`multisig`	-1.77 K	+1_860	26.79 K	56_756
`proxy`			3.81 K	2_629
`rand-extension`			5.12 K	6_654
`subber`			2.96 K
`trait-erc20`	-0.42 K	+15	10.10 K	10_659
`trait-flipper`			1.53 K	342
`trait-incrementer`			1.62 K	680

Link to the run | Last update: Wed Feb 2 02:13:45 CET 2022

cmichi · 2022-02-01T10:03:54Z

crates/lang/macro/src/lib.rs

+///     # // doesn't compile.
+///     # //
+///     # // On a sidenote, I think that with the removal of the `collections` module we'll be able
+///     # // to get rid of all the dynamic storage allocation infrastructure.


Just put all of this into an issue.

Yep made #1120

cmichi · 2022-02-01T10:06:03Z

Why is it necessary to remove lazy? E.g. the example with a Vec<T> in storage and trying to avoid it being loaded eagerly seems like a super-plausible case for it.

athei · 2022-02-01T10:08:03Z

Because of the interaction with initialize_contract: If you have a Lazy and don't overwrite it after calling initialize_contract you are fucked.

cmichi

If you move the comments into individual GitHub issues I'm fine with it. It's super-sad that we have to hide those collections for the moment, but we can always bring them back in later versions.

codecov-commenter · 2022-02-02T01:14:05Z

Codecov Report

Merging #1111 (bbad42b) into master (903abeb) will increase coverage by 15.12%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master    #1111       +/-   ##
===========================================
+ Coverage   63.69%   78.82%   +15.12%     
===========================================
  Files         252      252               
  Lines        9395     9397        +2     
===========================================
+ Hits         5984     7407     +1423     
+ Misses       3411     1990     -1421

Impacted Files	Coverage Δ
crates/lang/macro/src/lib.rs	`100.00% <ø> (ø)`
...lang/tests/ui/contract/pass/example-erc20-works.rs	`1.51% <0.00%> (ø)`
...ang/tests/ui/contract/pass/example-erc721-works.rs	`0.67% <ø> (ø)`
crates/storage/src/alloc/allocation.rs	`82.14% <ø> (ø)`
crates/storage/src/alloc/mod.rs	`75.00% <ø> (ø)`
crates/storage/src/collections/binary_heap/mod.rs	`100.00% <ø> (ø)`
crates/storage/src/collections/vec/mod.rs	`97.29% <ø> (ø)`
crates/allocator/src/bump.rs	`62.50% <0.00%> (-4.17%)`	⬇️
crates/lang/ir/src/ir/attrs.rs	`82.27% <0.00%> (+3.60%)`	⬆️
...ates/storage/src/collections/hashmap/fuzz_tests.rs	`100.00% <0.00%> (+4.25%)`	⬆️
... and 46 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 903abeb...bbad42b. Read the comment docs.

athei

Yeah let's keep Memory around for now. We can always remove it later since it doesn't touch storage.

* Hide top level `collections` and `lazy` modules * Ignore failing doc-tests These use types from `collections` which are not available publicly anymore. * Stop using `Lazy` in `ERC-20` example * Use `Mapping` re-export in `ERC-721` example * Update UI tests to match updated examples * Allow `dead_code` in `lazy` and `collections` modules * Remove `Box` and `Pack` from top level `ink_storage` re-exports * Stop using `Lazy` in `multisig` example * Remove `Lazy` usage from DNS example * Update path of `Mapping` in `ERC-1155` example * Stop using `Lazy` in `trait-erc20` example * Undo changes to `alloc` tests * Add TODOs noting why we're ignoring certain doctests * Remove references to `storage::Box` from doc comments * Stop re-exporting `boxed::Box` * Remove storage collection benches It doesn't looks like there's a way to keep these compiling if we're hiding the collection module, so the next best thing to do is to remove them completely. * Remove `Lazy` from `delegator` example * Appease Clippy * Mention tracking issue for code removal instead of blank TODO

HCastano added 16 commits January 21, 2022 10:24

Hide top level collections and lazy modules

f38b072

Ignore failing doc-tests

5d3f435

These use types from `collections` which are not available publicly anymore.

Stop using Lazy in ERC-20 example

489b315

Use Mapping re-export in ERC-721 example

7484163

Update UI tests to match updated examples

3cc44c7

Allow dead_code in lazy and collections modules

65b1877

Remove Box and Pack from top level ink_storage re-exports

af2d9f3

Stop using Lazy in multisig example

e6f4277

Remove Lazy usage from DNS example

dccc6cd

Update path of Mapping in ERC-1155 example

1766fa1

Stop using Lazy in trait-erc20 example

761fdc5

Undo changes to alloc tests

044a121

Add TODOs noting why we're ignoring certain doctests

54d3424

Remove references to storage::Box from doc comments

70bee28

Stop re-exporting boxed::Box

65c9865

Remove storage collection benches

108ee2c

It doesn't looks like there's a way to keep these compiling if we're hiding the collection module, so the next best thing to do is to remove them completely.

HCastano requested review from ascjones, cmichi and Robbepop as code owners January 25, 2022 05:54

Remove Lazy from delegator example

b5e49d2

HCastano requested a review from athei January 25, 2022 06:00

Appease Clippy

92ea5a0

athei reviewed Jan 28, 2022

View reviewed changes

cmichi reviewed Feb 1, 2022

View reviewed changes

cmichi approved these changes Feb 1, 2022

View reviewed changes

Merge branch 'master' into hc-hide-lazy-and-collection-modules

30e7b7e

This was referenced Feb 2, 2022

Remove collections and lazy module code #1119

Closed

Remove dynamic storage allocator #1120

Closed

Mention tracking issue for code removal instead of blank TODO

bbad42b

HCastano requested a review from athei February 2, 2022 00:39

athei approved these changes Feb 2, 2022

View reviewed changes

HCastano merged commit 927251c into master Feb 2, 2022

HCastano deleted the hc-hide-lazy-and-collection-modules branch February 2, 2022 22:39

HCastano mentioned this pull request Feb 3, 2022

Make Vector usable when initialized with SpreadAllocate #1070

Closed

This was referenced Feb 4, 2022

Port storage::BTreeMap to storage2 #414

Closed

Add storage2::HashMap::remove method #412

Closed

HCastano mentioned this pull request Feb 7, 2022

Review Existing Storage Type Sizes #996

Closed

cmichi mentioned this pull request Feb 8, 2022

Adapt docs to hidden lazy and collections use-ink/ink-docs#42

Closed

This was referenced Feb 9, 2022

Doc bug: Data Structures Overview page links 404 #1128

Closed

Remove Lazy in storage items OpenEmojiBattler/open-emoji-battler#78

Closed

Update to reflect collections and lazy removal use-ink/ink-docs#44

Merged

athei mentioned this pull request Feb 16, 2022

Macro based storage rework #1134

Closed

HCastano mentioned this pull request Feb 26, 2022

Remove dynamic storage allocator #1148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide `collection` and `lazy` modules #1111

Hide `collection` and `lazy` modules #1111

HCastano commented Jan 25, 2022

athei left a comment

athei Jan 28, 2022

HCastano Jan 28, 2022

cmichi Feb 1, 2022

HCastano commented Jan 28, 2022

paritytech-cicd-pr commented Jan 28, 2022 •

edited

Loading

cmichi Feb 1, 2022

HCastano Feb 2, 2022

cmichi commented Feb 1, 2022

athei commented Feb 1, 2022

cmichi left a comment

codecov-commenter commented Feb 2, 2022

athei left a comment

Hide collection and lazy modules #1111

Hide collection and lazy modules #1111

Conversation

HCastano commented Jan 25, 2022

Rational

Eager Loading

Compatibility

Follow-Ups

athei left a comment

Choose a reason for hiding this comment

athei Jan 28, 2022

Choose a reason for hiding this comment

HCastano Jan 28, 2022

Choose a reason for hiding this comment

cmichi Feb 1, 2022

Choose a reason for hiding this comment

HCastano commented Jan 28, 2022

paritytech-cicd-pr commented Jan 28, 2022 • edited Loading

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

cmichi Feb 1, 2022

Choose a reason for hiding this comment

HCastano Feb 2, 2022

Choose a reason for hiding this comment

cmichi commented Feb 1, 2022

athei commented Feb 1, 2022

cmichi left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 2, 2022

Codecov Report

athei left a comment

Choose a reason for hiding this comment

Hide `collection` and `lazy` modules #1111

Hide `collection` and `lazy` modules #1111

paritytech-cicd-pr commented Jan 28, 2022 •

edited

Loading