Simple Mapping Storage Primitive #946

HCastano · 2021-09-29T21:45:14Z

An implementation of what's described in #945.

~~Some things I'm still unsure about:~~

1. The handling of Mapping's key field
Not sure if this should be user initialized since it's kinda an implementation detail
which is there to make sure we don't have any collisions in storage.

2. The SpreadLayout implementation
So from what I can tell, when we implement this trait we're supposed to indicate how our
structure should be represented in the contract storage. In the case of Mapping, since
it doesn't actually own any data, its really just a proxy for how we represent Key in
storage. Am I thinking about this correctly?

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

HCastano · 2021-09-30T20:36:22Z

So this PR isn't quite ready yet, but I'm still at a point where I can use Mapping in the ERC-20 example and have it compile (this doesn't necessarily means it runs correctly on chain). The size improvements are great:

Master:  Original wasm size: 68.0K, Optimized: 28.8K
This PR: Original wasm size: 37.8K, Optimized: 12.6K

They're basically the same as @xgreenx's SimpleHashMap implementation, although this isn't surprising because our implementations are really similar. Gonna need to do some more thinking about where we can lose some extra bytes 😉

xgreenx · 2021-09-30T21:00:33Z

You are not supporting len and your storage key is not optional, so your implementation takes less space=) From ERC-20 example you can remove usage of Lazy and it will save 1.3 KB=)

I think it is better to return an Option<V> from the get method because in this case, the developer will be available to check if the object exists or not. Because in Solidity in some cases it was a headache.

Robbepop · 2021-10-04T16:56:52Z

Compiling the adjusted ERC-20 with cargo contract build yields 12.6kB.
Exchanging the Balance type from u128 to u64 or u32 we get down to 12.1kB.
Exchanging the AccountId from [u8; 32] to [u8; 8] we get down to 9.7kB.

This means that a majority of the cluft comes from copying the huge AccountId type around.
I see that especially in calls to the allowance function that takes 2 parameters by value and turns them into a tuple-packed reference.

Ideally we can introduce some traits in order to allow for use cases like these without requiring value types anywhere.
E.g. making &(U, V) equal to (&U, &V) in those cases so we could avoid a copy there.

edit:
With a minor tweak to the current implementation and setting Balance to u32 and AccountId to [u8; 4] (32 bit) the generated Wasm is able to efficiently pack the &(AccountId, Balance) expression leading to a Wasm file size of just 8.6kB. This underlines that the actual offender in Wasm file size is the huge AccountId. If we succeed in using references (32 bit on Wasm) more in places where we currently handling around AccountId instances I think we can win a lot of ground.

Robbepop

The Mapping is missing some trivial Debug implementation:

impl<K, V> core::fmt::Debug for Mapping<K, V> {
    fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
        f.debug_struct("Mapping").finish()
    }
}

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

* Add `Mapping` storage collection * Implement `insert` and `get` for `Mapping` * Implement `SpreadLayout` for `Mapping` * Fix typo * Add some basic tests * Fix some documentation formatting * Use `PackedLayout` as trait bound instead of `Encode/Decode` * Avoid using low level `ink_env` functions when interacting with storage * RustFmt * Appease Clippy * Only use single `PhantomData` field * Change `get` API to take reference to `key` * Implement `TypeInfo` and `StorageLayout` for `Mapping` * Properly gate `TypeInfo` and `StorageLayout` impls behind `std` * Replace `HashMap` with `Mapping` in ERC-20 example * Return `Option` from `Mapping::get` * Update ERC-20 to handle `Option` returns * Change `get` and `key` to use `Borrow`-ed values * Add `Debug` and `Default` implementations * Proper spelling * Change `insert` to only accept borrowed K,V pairs * Update ERC-20 example accordingly * Make more explicit what each `key` is referring to * Try using a `RefCell` instead of passing `Key` around * Try using `UnsafeCell` instead * Revert "Try using a `RefCell` instead of passing `Key` around" This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways. * Clean up some of the documentation * adjust the Mapping type for the new SpreadAllocate trait * adjust ERC-20 example for changes in Mapping type * remove commented out code * add doc comment to new_init * make it possible to use references in more cases with Mapping * use references in more cases for ERC-20 example contract * remove unnecessary references in Mapping methods * refactor/improve pull_packed_root_opt utility method slightly * fix ERC-20 example contract The problem with *self.total_supply is that it may implicitly read from storage in case it has not yet read a value from storage whereas Lazy::set just writes the value to the Lazy instance. Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Hernando Castano <[email protected]>

codecov-commenter · 2021-11-09T16:22:39Z

Codecov Report

Merging #946 (5e5a918) into master (c7cc828) will decrease coverage by 0.15%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
- Coverage   78.86%   78.71%   -0.16%     
==========================================
  Files         245      246       +1     
  Lines        9246     9260      +14     
==========================================
- Hits         7292     7289       -3     
- Misses       1954     1971      +17

Impacted Files	Coverage Δ
crates/storage/src/lazy/mod.rs	`85.29% <ø> (ø)`
crates/storage/src/traits/optspec.rs	`90.00% <66.66%> (-6.56%)`	⬇️
crates/storage/src/lazy/mapping.rs	`100.00% <100.00%> (ø)`
...tes/storage/src/collections/bitstash/fuzz_tests.rs	`40.00% <0.00%> (-60.00%)`	⬇️

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 c7cc828...5e5a918. Read the comment docs.

Robbepop · 2021-11-09T16:56:45Z

crates/lang/codegen/src/generator/storage.rs

@@ -97,6 +97,7 @@ impl Storage<'_> {
                derive(::ink_storage::traits::StorageLayout)
            )]
            #[derive(::ink_storage::traits::SpreadLayout)]
+            #[derive(::ink_storage::traits::SpreadAllocate)]


First: You cannot do this since not all #[ink(storage)] structs should implement SpreadAllocate which is why it is separate from SpreadLayout and has not been integrated into the already existing traits.
Second: If this was actually useful (which it isn't) it should reside in another PR.

For ink! codegen to automatically implement SpreadAllocate optionally we'd need support for Rust trivial trait bounds which currently are unstable.

My bad. I thought that the idea behind merging #995 before the Mapping was so we could derive SpreadAllocate for storage types in the codegen instead of manually deriving it

If Rust had trivial trait bounds we actually could do exactly that ... :(

HCastano · 2021-11-09T17:11:49Z

examples/erc20/lib.rs

@@ -59,20 +61,19 @@ mod erc20 {
        /// Creates a new ERC-20 contract with the specified initial supply.
        #[ink(constructor)]
        pub fn new(initial_supply: Balance) -> Self {
+            ink_lang::codegen::initialize_contract(|contract| Self::new_init(contract, initial_supply))


@Robbepop using the codegen module here feels wrong, is there a better way to expose this to users?

The actual plan is still that we provide some useful and well designed syntactic sugar so that this line of code won't be necessary but maybe that's not well thought through and maybe writing out this line of code actually isn't so bad. Then we could put this into some other module or into root.

HCastano · 2021-11-09T17:15:30Z

FYI, with the additional bloat from #998 we're currently sitting at 12.1K.

Robbepop · 2021-11-09T17:47:51Z

FYI, with the additional bloat from #998 we're currently sitting at 12.1K.

The bloat can easily be fixed with another PR and more intelligent codegen.

xgreenx · 2021-11-09T19:37:39Z

I tried to build this change and #999.

Erc20 with `Lazy` for `total_supply`:
Original wasm size: 34.1K, Optimized: 10.9K
Erc20 without `Lazy`:
Original wasm size: 33.6K, Optimized: 10.5K

xgreenx · 2021-11-10T20:36:01Z

Some other tests:

This + use-ink/cargo-contract#358: Original wasm size: 35.4K, Optimized: 11.7K
This + use-ink/cargo-contract#358 + #1012: Original wasm size: 34.3K, Optimized: 10.6K
This + #1012: Original wasm size: 35.2K, Optimized: 11.0K
This + #999 + use-ink/cargo-contract#358: Original wasm size: 33.2K, Optimized: 10.4K. In this case, #1012 doesn't cause any effect on Erc20.

HCastano · 2021-11-11T16:43:10Z

@xgreenx I haven't had time to loop back into this, so thanks for your experiments!

HCastano · 2021-11-11T17:50:15Z

I think we can probably merge this as-is, and fix anything in follow ups (like not using the codegen module directly). Having this in master would allow us to start doing more experiments with the Mapping

Robbepop · 2021-11-11T19:34:10Z

I would like to see the new Mapping type under ink_storage::lazy instead of ink_storage::collections module before we merge this.

Robbepop · 2021-11-14T17:16:16Z

crates/storage/src/lazy/mapping.rs

+    push_packed_root,
+    ExtKeyPtr,
+    KeyPtr,
+    PackedLayout,


* Add `Mapping` storage collection * Implement `insert` and `get` for `Mapping` * Implement `SpreadLayout` for `Mapping` * Fix typo * Add some basic tests * Fix some documentation formatting * Use `PackedLayout` as trait bound instead of `Encode/Decode` * Avoid using low level `ink_env` functions when interacting with storage * RustFmt * Appease Clippy * Only use single `PhantomData` field * Change `get` API to take reference to `key` * Implement `TypeInfo` and `StorageLayout` for `Mapping` * Properly gate `TypeInfo` and `StorageLayout` impls behind `std` * Replace `HashMap` with `Mapping` in ERC-20 example * Return `Option` from `Mapping::get` * Update ERC-20 to handle `Option` returns * Change `get` and `key` to use `Borrow`-ed values * Add `Debug` and `Default` implementations * Proper spelling * Change `insert` to only accept borrowed K,V pairs * Update ERC-20 example accordingly * Make more explicit what each `key` is referring to * Try using a `RefCell` instead of passing `Key` around * Try using `UnsafeCell` instead * Revert "Try using a `RefCell` instead of passing `Key` around" This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways. * Clean up some of the documentation * Simple Mapping type improvements (use-ink#979) * Add `Mapping` storage collection * Implement `insert` and `get` for `Mapping` * Implement `SpreadLayout` for `Mapping` * Fix typo * Add some basic tests * Fix some documentation formatting * Use `PackedLayout` as trait bound instead of `Encode/Decode` * Avoid using low level `ink_env` functions when interacting with storage * RustFmt * Appease Clippy * Only use single `PhantomData` field * Change `get` API to take reference to `key` * Implement `TypeInfo` and `StorageLayout` for `Mapping` * Properly gate `TypeInfo` and `StorageLayout` impls behind `std` * Replace `HashMap` with `Mapping` in ERC-20 example * Return `Option` from `Mapping::get` * Update ERC-20 to handle `Option` returns * Change `get` and `key` to use `Borrow`-ed values * Add `Debug` and `Default` implementations * Proper spelling * Change `insert` to only accept borrowed K,V pairs * Update ERC-20 example accordingly * Make more explicit what each `key` is referring to * Try using a `RefCell` instead of passing `Key` around * Try using `UnsafeCell` instead * Revert "Try using a `RefCell` instead of passing `Key` around" This reverts commit cede033. Using `RefCell`/`UnsafeCell` doesn't reduce the contract size more than what we have now, and it introduced `unsafe` code. We believe the limiting factor here is the `Key` type definition anyways. * Clean up some of the documentation * adjust the Mapping type for the new SpreadAllocate trait * adjust ERC-20 example for changes in Mapping type * remove commented out code * add doc comment to new_init * make it possible to use references in more cases with Mapping * use references in more cases for ERC-20 example contract * remove unnecessary references in Mapping methods * refactor/improve pull_packed_root_opt utility method slightly * fix ERC-20 example contract The problem with *self.total_supply is that it may implicitly read from storage in case it has not yet read a value from storage whereas Lazy::set just writes the value to the Lazy instance. Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Hernando Castano <[email protected]> * Use new `initialize_contract()` function * Derive `SpreadAllocate` for `ink(storage)` structs * Stop manually implementing SpreadAllocate for ERC-20 * Stop implementing `SpreadAllocate` in the storage codegen * Derive `SpreadAllocate` manually for ERC-20 * RustFmt example * Move `Mapping` from `collections` to `lazy` * Remove extra `0` in docs Co-authored-by: Robin Freyler <[email protected]>

HCastano added 6 commits September 29, 2021 11:01

Add Mapping storage collection

61d564d

Implement insert and get for Mapping

8ae8047

Implement SpreadLayout for Mapping

3a9d97e

Fix typo

3e29a25

Add some basic tests

d1387f7

Fix some documentation formatting

9e48c18

HCastano added the A-ink_storage [ink_storage] Work Item label Sep 29, 2021

HCastano requested a review from Robbepop September 29, 2021 21:45

Robbepop requested changes Sep 30, 2021

View reviewed changes

HCastano added 9 commits September 30, 2021 11:00

Use PackedLayout as trait bound instead of Encode/Decode

b7cf4dc

Avoid using low level ink_env functions when interacting with storage

dc87fba

RustFmt

728462a

Appease Clippy

5b2a8af

Only use single PhantomData field

a5d016e

Change get API to take reference to key

ad524c6

Implement TypeInfo and StorageLayout for Mapping

e6293a3

Properly gate TypeInfo and StorageLayout impls behind std

b55a24e

Replace HashMap with Mapping in ERC-20 example

9c6f853

HCastano added 3 commits October 1, 2021 10:48

Return Option from Mapping::get

299ba86

Update ERC-20 to handle Option returns

78e69f3

Merge branch 'master' into hc-simple-mapping-primitive

00ce6ea

Robbepop reviewed Oct 4, 2021

View reviewed changes

crates/storage/src/collections/mapping/mod.rs Outdated Show resolved Hide resolved

crates/storage/src/collections/mapping/mod.rs Outdated Show resolved Hide resolved

crates/storage/src/collections/mapping/mod.rs Outdated Show resolved Hide resolved

Robbepop reviewed Oct 4, 2021

View reviewed changes

crates/storage/src/collections/mapping/mod.rs Outdated Show resolved Hide resolved

crates/storage/src/collections/mapping/mod.rs Outdated Show resolved Hide resolved

crates/storage/src/collections/mapping/mod.rs Outdated Show resolved Hide resolved

HCastano added 3 commits October 11, 2021 11:30

Merge branch 'master' into hc-simple-mapping-primitive

a43e0de

Change get and key to use Borrow-ed values

7c55dbf

Add Debug and Default implementations

84576ed

HCastano and others added 4 commits November 4, 2021 18:35

Merge branch 'master' into hc-simple-mapping-primitive

2ed985f

Merge branch 'master' into hc-simple-mapping-primitive

7e55e56

Merge branch 'master' into hc-simple-mapping-primitive

507cf5b

HCastano added 3 commits November 9, 2021 17:31

Use new initialize_contract() function

c23e679

Derive SpreadAllocate for ink(storage) structs

334a9da

Stop manually implementing SpreadAllocate for ERC-20

408bf81

Robbepop requested changes Nov 9, 2021

View reviewed changes

HCastano added 2 commits November 9, 2021 18:08

Stop implementing SpreadAllocate in the storage codegen

b625a0a

Derive SpreadAllocate manually for ERC-20

2184cd6

HCastano commented Nov 9, 2021

View reviewed changes

RustFmt example

1854dcf

ascjones mentioned this pull request Nov 11, 2021

Reduce Wasm code bloat of emitting ink! events #990

Open

HCastano marked this pull request as ready for review November 11, 2021 17:49

HCastano added 2 commits November 12, 2021 11:29

Move Mapping from collections to lazy

c4a951c

Remove extra 0 in docs

5e5a918

HCastano requested a review from Robbepop November 12, 2021 10:34

Robbepop approved these changes Nov 14, 2021

View reviewed changes

crates/storage/src/lazy/mapping.rs

push_packed_root,

ExtKeyPtr,

KeyPtr,

PackedLayout,

Copy link

Collaborator

Robbepop Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

HCastano merged commit 7432565 into master Nov 15, 2021

HCastano deleted the hc-simple-mapping-primitive branch November 15, 2021 10:59

Robbepop mentioned this pull request Nov 19, 2021

Simple Mapping Storage Primitive #945

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple Mapping Storage Primitive #946

Simple Mapping Storage Primitive #946

HCastano commented Sep 29, 2021 •

edited

Loading

HCastano commented Sep 30, 2021

xgreenx commented Sep 30, 2021

Robbepop commented Oct 4, 2021 •

edited

Loading

Robbepop left a comment

codecov-commenter commented Nov 9, 2021 •

edited

Loading

Robbepop Nov 9, 2021

Robbepop Nov 9, 2021

HCastano Nov 9, 2021

Robbepop Nov 9, 2021

HCastano Nov 9, 2021

Robbepop Nov 9, 2021

HCastano commented Nov 9, 2021

Robbepop commented Nov 9, 2021

xgreenx commented Nov 9, 2021 •

edited

Loading

xgreenx commented Nov 10, 2021

HCastano commented Nov 11, 2021

HCastano commented Nov 11, 2021

Robbepop commented Nov 11, 2021

Robbepop Nov 14, 2021

Simple Mapping Storage Primitive #946

Simple Mapping Storage Primitive #946

Conversation

HCastano commented Sep 29, 2021 • edited Loading

HCastano commented Sep 30, 2021

xgreenx commented Sep 30, 2021

Robbepop commented Oct 4, 2021 • edited Loading

Robbepop left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 9, 2021 • edited Loading

Codecov Report

Robbepop Nov 9, 2021

Choose a reason for hiding this comment

Robbepop Nov 9, 2021

Choose a reason for hiding this comment

HCastano Nov 9, 2021

Choose a reason for hiding this comment

Robbepop Nov 9, 2021

Choose a reason for hiding this comment

HCastano Nov 9, 2021

Choose a reason for hiding this comment

Robbepop Nov 9, 2021

Choose a reason for hiding this comment

HCastano commented Nov 9, 2021

Robbepop commented Nov 9, 2021

xgreenx commented Nov 9, 2021 • edited Loading

xgreenx commented Nov 10, 2021

HCastano commented Nov 11, 2021

HCastano commented Nov 11, 2021

Robbepop commented Nov 11, 2021

Robbepop Nov 14, 2021

Choose a reason for hiding this comment

HCastano commented Sep 29, 2021 •

edited

Loading

Robbepop commented Oct 4, 2021 •

edited

Loading

codecov-commenter commented Nov 9, 2021 •

edited

Loading

xgreenx commented Nov 9, 2021 •

edited

Loading