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

remote-externalities: 'instant' snapshots, threading refactor, better progress logs #14057

Merged
merged 21 commits into from
May 5, 2023

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented May 1, 2023

Instant(ish) snapshot loading

  • The existing snapshots contain key / values without the trie. Loading the key/values can take a while even when batch inserting because the trie needs to be re-computed.
  • In this PR, the snapshot was refactored to contain the raw trie itself, and be loaded directly into the underlying TrieBackend storage that backs TestExternalties

Snapshot load performance (Macbook Pro M2 Max)

Chain Build Before After Improvement
Kusama debug 12.13s 0.88s 92.75%
Kusama release 1.93s 0.18s 90.67%
Polkadot debug 59.08s 2.66s 95.50%
Polkadot release 8.25s 0.44s 94.67%

Removes threading

  • Uses async/await futures for parallel requests instead of threads
  • Closes try-runtime-cli panics on Polkadot #14006
  • It makes loading state from a live node a little bit slower because it waits until all data has been downloaded before loading it into the trie. However, the degradation is small, and it should be encouraged to use snapshots instead when performance is required especially now that we have much faster snapshot loading.
  • Bonus: simplifies a lot of complex code

Better progress logs

Kusama on_runtime_upgrade live logs

Screen.Recording.2023-05-03.at.12.11.20.mp4

TODO Check pulling data from these remotes works:

  • wss://polkadot-try-runtime-node.parity-chains.parity.io:443
  • wss://kusama-try-runtime-node.parity-chains.parity.io:443
  • wss://westend-try-runtime-node.parity-chains.parity.io:443

@liamaharon liamaharon added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels May 1, 2023
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice, LGTM

Can you add an example of the changed CLI output in the PR description so the reviewers can a feeling how output will look like after this change?

@liamaharon liamaharon marked this pull request as draft May 2, 2023 08:56
@liamaharon
Copy link
Contributor Author

Converting this to a draft, I'd like to include a switch to inserting keys directly into OverlayedChanges into this PR.

@liamaharon liamaharon changed the title remote-externalities: threading and progress indicator refactor remote-externalities: threading refactor, logs overhaul and performance improvement May 2, 2023
@liamaharon liamaharon added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 2, 2023
@ggwpez
Copy link
Member

ggwpez commented May 2, 2023

Tested it for all four parity nodes and it works with this patch, thanks!

@liamaharon liamaharon added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label May 3, 2023
@liamaharon liamaharon changed the title remote-externalities: threading refactor, logs overhaul and 'instant' loading snapshots remote-externalities: threading refactor, logs overhaul and 'instant' snapshots May 3, 2023
@liamaharon liamaharon changed the title remote-externalities: threading refactor, logs overhaul and 'instant' snapshots remote-externalities: threading refactor, better progress logs, and 'instant' snapshots May 3, 2023
@liamaharon liamaharon added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 3, 2023
@liamaharon liamaharon changed the title remote-externalities: threading refactor, better progress logs, and 'instant' snapshots remote-externalities: threading refactor, better progress logs, 'instant' snapshots May 3, 2023
@liamaharon liamaharon changed the title remote-externalities: threading refactor, better progress logs, 'instant' snapshots remote-externalities: 'instant' snapshots, threading refactor, better progress logs May 3, 2023
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

It looks very promising!

I think now's a good time to revisit https://github.com/paritytech/devops/issues/2083 and its importance as well.

With the end goal of making this usable (in a stable and fast way 🙈) in our CI, it would make a lot of sense to have snapshots ready in our infra, and have the CI use those. With this fast loading time, it would be a smooth process.

@ggwpez
Copy link
Member

ggwpez commented May 4, 2023

With the end goal of making this usable (in a stable and fast way ) in our CI, it would make a lot of sense to have snapshots ready in our infra, and have the CI use those. With this fast loading time, it would be a smooth process.

It is pretty fast now. Takes about 3 minutes locally and 15 in CI.
I think we should rather focus on stability and add tests instead of optimizing more.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks!

What is this file utils/frame/remote-externalities/test_data/proxy_test?

primitives/state-machine/src/testing.rs Outdated Show resolved Hide resolved
primitives/state-machine/src/testing.rs Outdated Show resolved Hide resolved
utils/frame/remote-externalities/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/remote-externalities/src/lib.rs Show resolved Hide resolved
@liamaharon
Copy link
Contributor Author

@ggwpez utils/frame/remote-externalities/test_data/proxy_test is a snapshot file used in a test. I needed to update it to use the new snapshot format.

@liamaharon liamaharon requested a review from ggwpez May 4, 2023 16:24
@liamaharon liamaharon merged commit fb98ee1 into master May 5, 2023
@liamaharon liamaharon deleted the liam-remote-externalities-refactor branch May 5, 2023 07:15
niklasad1 pushed a commit that referenced this pull request Jun 19, 2023
…ress logs (#14057)

* remote externalities refactor

* remove redundant logs

* use const for parallel requests

* prefer functional

* improve variable naming

* handle requests error

* use overlayedchanges

* Revert "use overlayedchanges"

This reverts commit c0ddb87.

* Revert "Revert "use overlayedchanges""

This reverts commit 1d49362.

* Revert "Revert "Revert "use overlayedchanges"""

This reverts commit 06df786.

* backup/load raw storage values

* test raw storage drain and restore

* update snapshot tests

* improve logs

* clippy suggestions

* address comments

* fix example

* fix test

* clippy
coderobe pushed a commit that referenced this pull request Jun 20, 2023
…ress logs (#14057)

* remote externalities refactor

* remove redundant logs

* use const for parallel requests

* prefer functional

* improve variable naming

* handle requests error

* use overlayedchanges

* Revert "use overlayedchanges"

This reverts commit c0ddb87.

* Revert "Revert "use overlayedchanges""

This reverts commit 1d49362.

* Revert "Revert "Revert "use overlayedchanges"""

This reverts commit 06df786.

* backup/load raw storage values

* test raw storage drain and restore

* update snapshot tests

* improve logs

* clippy suggestions

* address comments

* fix example

* fix test

* clippy
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ress logs (paritytech#14057)

* remote externalities refactor

* remove redundant logs

* use const for parallel requests

* prefer functional

* improve variable naming

* handle requests error

* use overlayedchanges

* Revert "use overlayedchanges"

This reverts commit c0ddb87.

* Revert "Revert "use overlayedchanges""

This reverts commit 1d49362.

* Revert "Revert "Revert "use overlayedchanges"""

This reverts commit 06df786.

* backup/load raw storage values

* test raw storage drain and restore

* update snapshot tests

* improve logs

* clippy suggestions

* address comments

* fix example

* fix test

* clippy
vanderian pushed a commit to gasp-xyz/substrate that referenced this pull request Jul 26, 2023
…ress logs (paritytech#14057)

* remote externalities refactor

* remove redundant logs

* use const for parallel requests

* prefer functional

* improve variable naming

* handle requests error

* use overlayedchanges

* Revert "use overlayedchanges"

This reverts commit c0ddb87.

* Revert "Revert "use overlayedchanges""

This reverts commit 1d49362.

* Revert "Revert "Revert "use overlayedchanges"""

This reverts commit 06df786.

* backup/load raw storage values

* test raw storage drain and restore

* update snapshot tests

* improve logs

* clippy suggestions

* address comments

* fix example

* fix test

* clippy
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. B0-silent Changes should not be mentioned in any release notes 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.

try-runtime-cli panics on Polkadot
4 participants