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

Provide a StorageVec datastructure #1995

Merged
merged 37 commits into from
Nov 30, 2023
Merged

Conversation

xermicus
Copy link
Contributor

@xermicus xermicus commented Nov 15, 2023

Summary

Provide a vector like data structure, built on top of Mapping.

This allows to retrieve elements from the vector and grow the vector without loading and pushing all elements.

Description

Alternative: #1997

I favor this implementation slightly over #1997. It is less code and adds less contract size. The drawback is that here we have no iterators and no Index. However, I don't think iterators are really required. Also, #1997 provides binary search, however the vector needs to be sorted and this is kind of impossible if it grows too large.

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

xermicus and others added 19 commits November 2, 2023 11:56
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e84a87c) 53.37% compared to head (1d9b7d2) 53.84%.

Files Patch % Lines
crates/storage/src/lazy/vec.rs 97.26% 2 Missing ⚠️
crates/storage/src/lazy/mod.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1995      +/-   ##
==========================================
+ Coverage   53.37%   53.84%   +0.47%     
==========================================
  Files         220      221       +1     
  Lines        6884     6957      +73     
  Branches        0     3068    +3068     
==========================================
+ Hits         3674     3746      +72     
- Misses       3210     3211       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Nov 17, 2023

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

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

Contract Upstream Size (kB) PR Size (kB) Diff (kB) Diff (%) Change
basic-contract-caller 2.967 2.967 0 0
basic-contract-caller/other-contract 1.337 1.337 0 0
call-builder-return-value 8.735 8.735 0 0
call-runtime 1.769 1.769 0 0
conditional-compilation 1.209 1.209 0 0
contract-storage 7.171 7.171 0 0
contract-terminate 1.092 1.092 0 0
contract-transfer 1.444 1.444 0 0
custom-allocator 7.428 7.428 0 0
dns 7.249 7.249 0 0
e2e-call-runtime 1.058 1.058 0 0
e2e-runtime-only-backend 1.635 1.635 0 0
erc1155 13.968 13.962 -0.006 -0.0429553 📉
erc20 6.687 6.687 0 0
erc721 9.64 9.64 0 0
events 4.763 4.763 0 0
flipper 1.393 1.393 0 0
incrementer 1.221 1.221 0 0
lang-err-integration-tests/call-builder-delegate 2.317 2.317 0 0
lang-err-integration-tests/call-builder 4.847 4.847 0 0
lang-err-integration-tests/constructors-return-value 1.773 1.773 0 0
lang-err-integration-tests/contract-ref 4.328 4.328 0 0
lang-err-integration-tests/integration-flipper 1.571 1.571 0 0
mapping-integration-tests 7.685 7.685 0 0
mother 12.544 12.589 0.045 0.358737 📈
multi-contract-caller 5.924 5.924 0 0
multi-contract-caller/accumulator 1.095 1.095 0 0
multi-contract-caller/adder 1.669 1.669 0 0
multi-contract-caller/subber 1.689 1.689 0 0
multisig 21.471 21.471 0 0
payment-channel 5.501 5.501 0 0
sr25519-verification 0.865 0.865 0 0
static-buffer 1.405 1.405 0 0
trait-dyn-cross-contract-calls 2.466 2.466 0 0
trait-dyn-cross-contract-calls/contracts/incrementer 1.305 1.305 0 0
trait-erc20 7.063 7.063 0 0
trait-flipper 1.209 1.209 0 0
trait-incrementer 1.37 1.37 0 0
upgradeable-contracts/delegator 2.908 2.908 0 0
upgradeable-contracts/delegator/delegatee 1.369 1.369 0 0
upgradeable-contracts/set-code-hash 1.464 1.464 0 0
upgradeable-contracts/set-code-hash/updated-incrementer 1.443 1.443 0 0
wildcard-selector 2.622 2.622 0 0

Link to the run | Last update: Thu Nov 30 13:09:55 CET 2023

@xermicus xermicus marked this pull request as ready for review November 17, 2023 13:09
@xermicus xermicus mentioned this pull request Nov 17, 2023
5 tasks
Signed-off-by: Cyrill Leutwiler <[email protected]>
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

I prefer this proposal over #1997 too. Simpler, smaller code size, and in line with the philosophy of Mapping and Lazy (thin wrappers around storage access).

crates/storage/src/lazy/vec.rs Outdated Show resolved Hide resolved
crates/storage/src/lazy/vec.rs Outdated Show resolved Hide resolved
crates/storage/src/lazy/vec.rs Show resolved Hide resolved
integration-tests/lazyvec-integration-test/lib.rs Outdated Show resolved Hide resolved
@xgreenx
Copy link
Collaborator

xgreenx commented Nov 22, 2023

I agree with Andrew, and I like this approach more than a Rust-like vector with caching inside.

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

crates/storage/src/lazy/vec.rs Outdated Show resolved Hide resolved
Signed-off-by: Cyrill Leutwiler <[email protected]>
@xermicus
Copy link
Contributor Author

Yeah why not, 🪓 it

Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small nit

pub fn len(&self) -> u32 {
debug_assert!(self.len_cached.is_none() || self.len.get() == self.len_cached);

self.len_cached
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding here is that subsequent calls of len() will still call uncached len? Why not cache it here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For that, and all other getters it would need a &mut self receiver.

Maybe could get around that with interior mutability to remove the inefficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I missed that. Easy to do with a Cell though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SkymanOne added it together with a test

@xermicus xermicus changed the title Storage vec Provide a StorageVec datastructure Nov 29, 2023
Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

Maybe it makes sense to add a new method like swap_and_remove(index: usize) that swaps the element at index with the last element and removes it(via pop). It is very popular method to remove elements from the vector in Solidity

crates/storage/src/lazy/vec.rs Outdated Show resolved Hide resolved
@xermicus xermicus merged commit 9acc3c2 into use-ink:master Nov 30, 2023
21 checks passed
@xermicus
Copy link
Contributor Author

@xgreenx I see, Just merged it to get it into the RC, but can do a follow up for adding a swap remove API anytime :)

@SkymanOne SkymanOne mentioned this pull request Mar 4, 2024
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