Append overlay optimization. #1223

cheme · 2023-08-28T15:24:06Z

This branch propose to avoid clones in append by storing offset and size in previous overlay depth.
That way on rollback we can just truncate and change size of existing value.
To avoid copy it also means that :

append on new overlay layer if there is an existing value: create a new Append entry with previous offsets, and take memory of previous overlay value.
rollback on append: restore value by applying offsets and put it back in previous overlay value
commit on append: appended value overwrite previous value (is an empty vec as the memory was taken). offsets of commited layer are dropped, if there is offset in previous overlay layer they are maintained.
set value (or remove) when append offsets are present: current appended value is moved back to previous overlay value with offset applied and current empty entry is overwrite (no offsets kept).

The modify mechanism is not needed anymore.
This branch lacks testing and break some existing genericity (bit of duplicated code), but good to have to check direction.

Generally I am not sure if it is worth or we just should favor differents directions (transients blob storage for instance), as the current append mechanism is a bit tricky (having a variable length in first position means we sometime need to insert in front of a vector).

Fix #30.

cheme · 2024-01-15T15:23:52Z

@ core devs, this pr needs reviewers, anyone familiar with the commit overlay internals or wanted to be are very welcome.

substrate/primitives/state-machine/fuzz/fuzz_targets/fuzz_append.rs

substrate/primitives/state-machine/src/basic.rs

substrate/primitives/state-machine/src/overlayed_changes/mod.rs

substrate/primitives/state-machine/src/basic.rs

substrate/primitives/state-machine/src/lib.rs

substrate/primitives/state-machine/src/overlayed_changes/changeset.rs

This reverts commit bd3c28e.

bkchr · 2024-06-03T20:10:46Z

@ggwpez @skunert this is now ready for another pass by you. Then we can merge this.

ggwpez

My review is a bit shallow, but the fuzzer seems good.

ggwpez · 2024-06-04T13:41:43Z

substrate/primitives/state-machine/fuzz/Cargo.toml

+
+# Prevent this from interfering with workspaces
+[workspace]
+members = ["."]


How does it interfere with it? Do we not want to run it from the top level?

substrate/primitives/state-machine/src/fuzzing.rs

ggwpez · 2024-06-04T14:20:58Z

substrate/primitives/state-machine/src/overlayed_changes/changeset.rs

+		/// If `None`, than `data` is not yet prefixed with the length.
+		materialized_length: Option<u32>,
+		/// The size of `data` in the parent transactional layer.
+		parent_size: Option<usize>,


Is this none if there is no parent layer or if the values does not exist in the parent value?

substrate/primitives/state-machine/src/overlayed_changes/changeset.rs

ggwpez · 2024-06-04T21:12:07Z

substrate/primitives/state-machine/src/fuzzing.rs

+				}
+				self.transaction_depth -= 1;
+				self.reference.commit_transaction();
+				ext.storage_commit_transaction().unwrap();


It looks like the *_offchain functions did not really change, but maybe still good to also test them?

skunert

Double checked the logic, looks good.

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

…eset.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]>

substrate/primitives/state-machine/src/overlayed_changes/changeset.rs

…eset.rs

paritytech-cicd-pr · 2024-06-11T21:30:47Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6443753

substrate/primitives/state-machine/src/overlayed_changes/changeset.rs

…eset.rs

This branch propose to avoid clones in append by storing offset and size in previous overlay depth. That way on rollback we can just truncate and change size of existing value. To avoid copy it also means that : - append on new overlay layer if there is an existing value: create a new Append entry with previous offsets, and take memory of previous overlay value. - rollback on append: restore value by applying offsets and put it back in previous overlay value - commit on append: appended value overwrite previous value (is an empty vec as the memory was taken). offsets of commited layer are dropped, if there is offset in previous overlay layer they are maintained. - set value (or remove) when append offsets are present: current appended value is moved back to previous overlay value with offset applied and current empty entry is overwrite (no offsets kept). The modify mechanism is not needed anymore. This branch lacks testing and break some existing genericity (bit of duplicated code), but good to have to check direction. Generally I am not sure if it is worth or we just should favor differents directions (transients blob storage for instance), as the current append mechanism is a bit tricky (having a variable length in first position means we sometime need to insert in front of a vector). Fix #30. --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: EgorPopelyaev <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: joe petrowski <[email protected]> Co-authored-by: Liam Aharon <[email protected]> Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Sebastian Kunert <[email protected]>

kianenigma · 2024-06-13T02:49:50Z

Would any of the approvers or the new/old author be able to provide a TLDR if this has any ramifications in the FRAME code that we should be aware of? Is there anything that was previously an anti-pattern, and is now possible? The original PR description seems dated to me.

cheme · 2024-06-13T06:48:59Z

From where the PR was last time I touch it (I don't think there was design changes): For a given key:

never mix append and insert operations
append only operation (no read) got linear cost
read can have an associated cost (producing a vec from all the appended element). IIRC there is a corner case where materializing read that involves shifting bytes due to resizing the scale encoded size. Apart from that read should be rather fine (for instance moving the encoded number of element at the end of the entry could fix it, thinking about it could also use a vecdeque but this is still a corner case).

ggwpez · 2024-06-13T11:55:19Z

Would any of the approvers or the new/old author be able to provide a TLDR if this has any ramifications in the FRAME code that we should be aware of? Is there anything that was previously an anti-pattern, and is now possible? The original PR description seems dated to me.

I think not in general. Having large vectors inside StorageValues is already an anti-pattern. We now just made that quicker in the append+revert case.

* master: (29 commits) Append overlay optimization. (#1223) finalization: Skip tree route calculation if no forks present (#4721) Remove unncessary call remove_from_peers_set (#4742) add pov-recovery unit tests and support for elastic scaling (#4733) approval-voting: Add no shows debug information (#4726) Revamp the Readme of the parachain template (#4713) Update README.md to move the PSVM link under a "Tooling" section under the "Releases" section (#4734) frame/proc-macro: Refactor code for better readability (#4712) Contracts: update wasmi to 0.32 (#3679) Backport style changes from P<>K bridge to R<>W bridge (#4732) New reference doc for Custom RPC V2 (#4654) Frame Pallets: Clean a lot of test setups (#4642) Fix occupied core handling (#4691) statement-distribution: Fix false warning (#4727) Update the README to include a link to the Polkadot SDK Version Manager (#4718) Cleanup PVF artifact by cache limit and stale time (#4662) Update link to a latest polkadot release (#4711) [CI] Delete cargo-deny config (#4677) fix build on MacOS: bump secp256k1 and secp256k1-sys to patched versions (#4709) Unify dependency aliases (#4633) ...

This branch propose to avoid clones in append by storing offset and size in previous overlay depth. That way on rollback we can just truncate and change size of existing value. To avoid copy it also means that : - append on new overlay layer if there is an existing value: create a new Append entry with previous offsets, and take memory of previous overlay value. - rollback on append: restore value by applying offsets and put it back in previous overlay value - commit on append: appended value overwrite previous value (is an empty vec as the memory was taken). offsets of commited layer are dropped, if there is offset in previous overlay layer they are maintained. - set value (or remove) when append offsets are present: current appended value is moved back to previous overlay value with offset applied and current empty entry is overwrite (no offsets kept). The modify mechanism is not needed anymore. This branch lacks testing and break some existing genericity (bit of duplicated code), but good to have to check direction. Generally I am not sure if it is worth or we just should favor differents directions (transients blob storage for instance), as the current append mechanism is a bit tricky (having a variable length in first position means we sometime need to insert in front of a vector). Fix #30. --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: EgorPopelyaev <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: joe petrowski <[email protected]> Co-authored-by: Liam Aharon <[email protected]> Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Sebastian Kunert <[email protected]>

This branch propose to avoid clones in append by storing offset and size in previous overlay depth. That way on rollback we can just truncate and change size of existing value. To avoid copy it also means that : - append on new overlay layer if there is an existing value: create a new Append entry with previous offsets, and take memory of previous overlay value. - rollback on append: restore value by applying offsets and put it back in previous overlay value - commit on append: appended value overwrite previous value (is an empty vec as the memory was taken). offsets of commited layer are dropped, if there is offset in previous overlay layer they are maintained. - set value (or remove) when append offsets are present: current appended value is moved back to previous overlay value with offset applied and current empty entry is overwrite (no offsets kept). The modify mechanism is not needed anymore. This branch lacks testing and break some existing genericity (bit of duplicated code), but good to have to check direction. Generally I am not sure if it is worth or we just should favor differents directions (transients blob storage for instance), as the current append mechanism is a bit tricky (having a variable length in first position means we sometime need to insert in front of a vector). Fix paritytech#30. --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: EgorPopelyaev <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: joe petrowski <[email protected]> Co-authored-by: Liam Aharon <[email protected]> Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Sebastian Kunert <[email protected]>

cheme added 2 commits August 28, 2023 17:21

append overlay optimization.

0466d8c

Merge branch 'master' into append

341987f

paritytech-ci requested review from a team August 28, 2023 15:28

cheme added A0-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix T0-node This PR/Issue is related to the topic “node”. I2-bug The node fails to follow expected behavior. labels Aug 28, 2023

cheme added 5 commits August 28, 2023 17:34

update lock

ac2b72c

no more companion

1fdd3c6

clippy

c94dde4

fmt

a022d9b

Merge branch 'master' into append

d8f21a0

cheme requested a review from koute as a code owner November 9, 2023 07:55

cheme added 6 commits November 9, 2023 09:00

license

e261624

Merge branch 'master' into append

31eebd3

lock update

0c9aedc

trying prdoc

5c44ad3

mut

9541b33

mut

70b94cc

cheme assigned bkchr and unassigned bkchr Jan 15, 2024

cheme requested a review from a team January 15, 2024 15:22

ggwpez reviewed Jan 15, 2024

View reviewed changes

cheme added 4 commits January 16, 2024 10:31

fuzzing in its own file.

b28527f

trying to return error

bd3c28e

Revert "trying to return error" not too sure about this direction

7b9face

This reverts commit bd3c28e.

minor enhance

3bdd2f0

bkchr approved these changes Jun 3, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into append

58013c9

skunert self-requested a review June 4, 2024 07:57

ggwpez approved these changes Jun 4, 2024

View reviewed changes

Add test and comment about append merge on commit

6fdfec0

skunert approved these changes Jun 11, 2024

View reviewed changes

bkchr and others added 3 commits June 11, 2024 22:48

Update substrate/primitives/state-machine/src/fuzzing.rs

adc8c83

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

Update substrate/primitives/state-machine/src/overlayed_changes/chang…

4561991

…eset.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]>

Merge branch 'master' into append

078e116

bkchr reviewed Jun 11, 2024

View reviewed changes

substrate/primitives/state-machine/src/overlayed_changes/changeset.rs Show resolved Hide resolved

Update substrate/primitives/state-machine/src/overlayed_changes/chang…

318dc62

…eset.rs

bkchr enabled auto-merge June 11, 2024 21:03

bkchr reviewed Jun 11, 2024

View reviewed changes

substrate/primitives/state-machine/src/overlayed_changes/changeset.rs Outdated Show resolved Hide resolved

Update substrate/primitives/state-machine/src/overlayed_changes/chang…

18513b3

…eset.rs

bkchr added this pull request to the merge queue Jun 11, 2024

Merged via the queue into paritytech:master with commit ad86209 Jun 11, 2024
153 of 157 checks passed

This was referenced Aug 21, 2024

Update polkadot-sdk from v1.11.0 to stable2407 moondance-labs/tanssi#659

Open

Update polkadot-sdk from v1.11.0 to stable2407 moonbeam-foundation/moonbeam#2912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append overlay optimization. #1223

Append overlay optimization. #1223

cheme commented Aug 28, 2023

cheme commented Jan 15, 2024 •

edited

Loading

bkchr commented Jun 3, 2024

ggwpez left a comment

ggwpez Jun 4, 2024

ggwpez Jun 4, 2024

ggwpez Jun 4, 2024 •

edited

Loading

skunert left a comment

paritytech-cicd-pr commented Jun 11, 2024

kianenigma commented Jun 13, 2024

cheme commented Jun 13, 2024

ggwpez commented Jun 13, 2024

Append overlay optimization. #1223

Append overlay optimization. #1223

Conversation

cheme commented Aug 28, 2023

cheme commented Jan 15, 2024 • edited Loading

bkchr commented Jun 3, 2024

ggwpez left a comment

Choose a reason for hiding this comment

ggwpez Jun 4, 2024

Choose a reason for hiding this comment

ggwpez Jun 4, 2024

Choose a reason for hiding this comment

ggwpez Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

skunert left a comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Jun 11, 2024

kianenigma commented Jun 13, 2024

cheme commented Jun 13, 2024

ggwpez commented Jun 13, 2024

cheme commented Jan 15, 2024 •

edited

Loading

ggwpez Jun 4, 2024 •

edited

Loading