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

Make wasm runtime cache size configurable#10177

Merged
bkchr merged 15 commits intoparitytech:masterfrom
moonbeam-foundation:elois-runtime-cache-size
Dec 9, 2021
Merged

Make wasm runtime cache size configurable#10177
bkchr merged 15 commits intoparitytech:masterfrom
moonbeam-foundation:elois-runtime-cache-size

Conversation

@librelois
Copy link
Copy Markdown
Contributor

@librelois librelois commented Nov 4, 2021

Polkadot companion: paritytech/polkadot#4466

What does it do?

Makes configurable the maximum number of different runtime versions that can be kept in cache.

What important points should reviewers know?

Before this PR, the cache had an array of 2 VersionedRuntime, with a custom implementation of a "Last Recent Usage" cache strategy which consisted in swapping a runtime in 1st position when it was used.
This custom implementation is not compliant if the cache contains more than 2 elements, then a full LRU algorithm is needed, that's why I use the lru crate which does this very well.

skip check-dependent-cumulus

@crystalin
Copy link
Copy Markdown
Contributor

@bkchr sorry to ping you, but not sure who can do a review of this one.

Copy link
Copy Markdown
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. I left some comments

@librelois
Copy link
Copy Markdown
Contributor Author

Sorry for the late review. I left some comments

Thanks for the review, I applied your feedback

Copy link
Copy Markdown
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

2 nitpicks, otherwise seems to look good.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Dec 1, 2021

It also doesn't compile.

@librelois
Copy link
Copy Markdown
Contributor Author

It also doesn't compile.

Fixed

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Dec 2, 2021

It also doesn't compile.

Fixed

There are still warnings. Cargo fmt is also not happy.

@librelois
Copy link
Copy Markdown
Contributor Author

There are still warnings. Cargo fmt is also not happy.

Sorry about that, it should be fine now.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Dec 3, 2021

There are still warnings. Cargo fmt is also not happy.

Sorry about that, it should be fine now.

I need to inform you, that CI is still not happy 🙈

@librelois
Copy link
Copy Markdown
Contributor Author

@bkchr It seems that the CI is green now. There is no need of companion PR on polkadot or cumulus.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Dec 3, 2021

@librelois for polkadot we need a companion. See the error. Cumulus should be fine and can be ignored.

@bkchr bkchr 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Dec 3, 2021
@bkchr bkchr requested a review from arkpar December 3, 2021 13:33
@librelois
Copy link
Copy Markdown
Contributor Author

@librelois for polkadot we need a companion. See the error. Cumulus should be fine and can be ignored.

Can one of you take care of the polkadot companion PR or should I do it myself? And in this last case, how to proceed? (I have never made a "polkadot companion" PR).

@arkpar
Copy link
Copy Markdown
Member

arkpar commented Dec 3, 2021

The docs should probably specify that --runtime-cache-size is capped at 255. Btw what happens if the users sets --runtime-cache-size=1000?

@librelois
Copy link
Copy Markdown
Contributor Author

The docs should probably specify that --runtime-cache-size is capped at 255. Btw what happens if the users sets --runtime-cache-size=1000?

runtime_cache_size is of type u8 so it is structopt that will fail to parse the value and will print an error to the user.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Dec 3, 2021

@librelois for polkadot we need a companion. See the error. Cumulus should be fine and can be ignored.

Can one of you take care of the polkadot companion PR or should I do it myself? And in this last case, how to proceed? (I have never made a "polkadot companion" PR).

You need to create a pr to polkadot that fixes the compilation with your branch. Then you need to mention it as polkadot companion: link in the description of this pr.

@librelois
Copy link
Copy Markdown
Contributor Author

You need to create a pr to polkadot that fixes the compilation with your branch. Then you need to mention it as polkadot companion: link in the description of this pr.

It's done

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Dec 4, 2021

@librelois can you please merge master into this branch? 🙈

@librelois
Copy link
Copy Markdown
Contributor Author

@bkchr Looking at the log of the gitlab-check-dependent-polkadot job, there seems to be a problem in your CI:

$ ./pipeline-scripts/check_dependent_project.sh paritytech substrate --substrate "$DEPENDENT_REPO" "$GITHUB_PR_TOKEN" "$CARGO_UPDATE_CRATES"
check_dependent_project
========================
This check ensures that this project's dependents do not suffer downstream breakages from new code
changes.
From https://gitlab.parity.io/parity/substrate
 * branch              master     -> FETCH_HEAD
fatal: It seems that there is already a rebase-apply directory, and
I wonder if you are in the middle of another rebase.  If that is the
case, please try
	git rebase (--continue | --abort | --skip)
If that is not the case, please
	rm -fr ".git/rebase-apply"
and run me again.  I am stopping in case you still have something
valuable there.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Dec 9, 2021

bot merge

@paritytech-processbot
Copy link
Copy Markdown

Error: Github API says "Allow edits from maintainers" is not enabled for paritytech/polkadot#4466. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.

@librelois
Copy link
Copy Markdown
Contributor Author

Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.

I don't have any checkboxes like "Allow edits from maintainers":

image

Maybe I need to redo the companion PR from a personal fork instead of the purestake fork.

@bkchr bkchr merged commit 1aea26e into paritytech:master Dec 9, 2021
@librelois librelois deleted the elois-runtime-cache-size branch December 9, 2021 15:10
seunlanlege pushed a commit to seunlanlege/substrate that referenced this pull request Dec 17, 2021
* Make wasm runtime cache size configurable

* apply review comments

* remove VersionedRuntimeValue

* fix compilation

* VersionedRuntime: replace clone by Arc

* fmt

* fix warnings

* fix tests compilation

* fmt
@FlorianFranzen
Copy link
Copy Markdown
Contributor

FlorianFranzen commented Dec 21, 2021

@librelois Would have been nice it that new parameter was also documented in code and that I did not have to read this issue to understand what it is for, e.g. see also the missing rust docs.

grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Make wasm runtime cache size configurable

* apply review comments

* remove VersionedRuntimeValue

* fix compilation

* VersionedRuntime: replace clone by Arc

* fmt

* fix warnings

* fix tests compilation

* fmt
aurexav added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 8, 2022
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Make wasm runtime cache size configurable

* apply review comments

* remove VersionedRuntimeValue

* fix compilation

* VersionedRuntime: replace clone by Arc

* fmt

* fix warnings

* fix tests compilation

* fmt
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.

5 participants