Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

frame_support::storage: Add StorageStreamIter #12721

Merged
merged 20 commits into from
Dec 17, 2022

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Nov 16, 2022

This pr adds the StorageStreamIter trait. This trait is currently only implemented for StorageValue which store "SCALE container type" e.g. Vec, BTreeMap etc. A SCALE container type is a type that follows the following encoding structure Compact<u32>(len) ++ #( item.encode() )*. The streaming iterator is a way to decode container values using an almost constant memory usage in contrast to decoding the entire value once into memory. The memory of a runtime is constrained, especially when it comes to a single allocation. This can be used for stuff like decoding events in an offchain worker or other huge data. It could probably also used for on chain operations, but will may require some more performance optimizations.

Internally this works by using sp_io::storage::read to read chunks from the state as requested by the decoder. As each call to read is a host function call there is some internal cache that currently stores 2048 bytes. This reduces the number of host calls and improves the performance of the iterator.

@bkchr bkchr added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Nov 16, 2022
frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
}
}

/// An iterator that streams values directly from storage.
Copy link
Member

Choose a reason for hiding this comment

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

We should also document that the value should not be modified as long as at least one Stream iter operates on it.

@bkchr bkchr requested review from koute and ggwpez November 28, 2022 02:03
Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

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

We could still make the errors and the code a little nicer, but functionally looks good to me.

frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
Comment on lines 179 to 180
if self.read >= self.length {
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Could reduce one level of indentation by doing an early exit.

if self.read >= self.length {
    return None;
}

frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
Comment on lines 301 to 303
let num_cached = self.buffer.len() - self.buffer_pos;

into[..num_cached].copy_from_slice(&self.buffer[self.buffer_pos..]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could simplify this a little bit with split_at_mut, e.g.

let (out_already_read, out_remaining) = into.split_at_mut(self.buffer.len() - self.buffer_pos);
out_already_read.copy_from_slice(&self.buffer[self.buffer_pos..]);

And then later:

		if let Some(length_minus_offset) =
			sp_io::storage::read(&self.key, &mut out_remaining, self.offset)
		{
			if length_minus_offset as usize < out_remaining.len() {
				return Err("Not enough data to fill the buffer".into())
			}

			self.ensure_total_length_did_not_change(length_minus_offset)?;
			self.offset += out_remaining.len() as u32;

			Ok(())
		} else {

frame/support/src/storage/stream_iter.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member Author

bkchr commented Dec 16, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for a0302c5

@bkchr
Copy link
Member Author

bkchr commented Dec 17, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 10629aa

@bkchr
Copy link
Member Author

bkchr commented Dec 17, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit a96cb57 into master Dec 17, 2022
@paritytech-processbot paritytech-processbot bot deleted the bkchr-storage-read branch December 17, 2022 09:07
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736/1

trusch added a commit to KILTprotocol/kilt-node that referenced this pull request Jan 23, 2023
## fixes KILTProtocol/ticket#2392

## Breaking Changes for us

~~None! 🥳~~

Edit: Forgot to also check with try-runtime feature enabled. There is a
small tweak necessary because of [This PR about
on-runtime-upgrade](paritytech/substrate#13045)

No database migrations, no runtime migrations and no new host functions.

## Polkadot Release Link

https://github.com/paritytech/polkadot/releases/tag/v0.9.37

## Release Analysis Forum Post

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736

## Cool new stuff that might be useful (or not)

* [frame_support::storage: Add
StorageStreamIter](paritytech/substrate#12721)
* If we have a StorageValue that contains something iterable, we can
directly iterate over it, without copying the memory first by a regular
get() call.
* [Add ensure_* mathematical
methods](paritytech/substrate#12754)
* The checked_* family of calls returns an Option which is in 99% of the
cases mapped to an error
* ensure_* calls directly return an error which can be propagated using
questionmark operator more easily
* [Kusama shows how to express complex local origins in XCM
messages](paritytech/polkadot#6273)
* Perhaps the most interesting one in this release, would be a good idea
for @weichweich and @ntn-x2 to have a look into this
* [pallet_uniques successor NFTv2 is out! 🥳 😄
](paritytech/substrate#12765)
* Finally we can have NFTs with owner controlled metadata on our chain.
* They even literally mention that this way users can write DIDs
directly on their NFT!
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
* Save

* Add some test

* Yep

* Move to its own file

* More work

* More

* Finish implementation

* Start resolving comments and fixing bugs

* Fix all review comments

* Update frame/support/src/storage/stream_iter.rs

Co-authored-by: Koute <koute@users.noreply.github.com>

* Update frame/support/src/storage/stream_iter.rs

Co-authored-by: Koute <koute@users.noreply.github.com>

* Review feedback

* FMT

* Okay, let's initialize the values...

* Fix...

Co-authored-by: Koute <koute@users.noreply.github.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Save

* Add some test

* Yep

* Move to its own file

* More work

* More

* Finish implementation

* Start resolving comments and fixing bugs

* Fix all review comments

* Update frame/support/src/storage/stream_iter.rs

Co-authored-by: Koute <koute@users.noreply.github.com>

* Update frame/support/src/storage/stream_iter.rs

Co-authored-by: Koute <koute@users.noreply.github.com>

* Review feedback

* FMT

* Okay, let's initialize the values...

* Fix...

Co-authored-by: Koute <koute@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants