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

Bump wasmtime default table_elements #11864

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jul 19, 2022

Some Polkadot tests started failing (paritytech/polkadot#5788) with:

Failed to get runtime version: cannot create module: table index 0 has a minimum element size of 2093 which exceeds the limit of 2048

Investigation showed that the --feature=runtime-benchmarks flag causes the increase of the WASM size.
Can I just bump this value or do the others need an increase as well?
The tests work with the increase to 3072.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez requested a review from koute July 19, 2022 17:37
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jul 19, 2022
@ggwpez ggwpez added 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 Jul 19, 2022
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

What other consequences does changing this have?

@koute
Copy link
Contributor

koute commented Jul 20, 2022

What other consequences does changing this have?

Slightly more memory usage. But it shouldn't really matter as it shouldn't be a huge amount. (If push comes to shove we can always just extract the exact value necessary from the WASM blob and use that as a limit, since we instantiate a separate wasmtime::Engine per runtime anyway.)

@ggwpez
Copy link
Member Author

ggwpez commented Jul 20, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 7b81072 into master Jul 20, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-bump-heap-pages branch July 20, 2022 08:50
@bkchr
Copy link
Member

bkchr commented Jul 20, 2022

@koute why not use the default number of table elements which is 10k? I have seen other teams having issues because of this number being to small.

@koute
Copy link
Contributor

koute commented Jul 20, 2022

@bkchr Originally I didn't want to overcomplicate the code with autodetecting the necessary value (since it'd require some extra changes as the precompiled codepath used by PVFs doesn't currently have direct access to the WASM blob so it can't extract the value) so I decided to just pick a number and hardcode it for now.

As far as the number itself is concerned I wanted to be conservative since the impact of this is AFAIK technically multiplicative (the number of table elements times the configured maximum number of instances times how many runtimes we keep ready for instantiation). So I looked at what we need in production and just went with a double of that number.

If anyone's having issues with this limit please don't hesitate to ping me; for sure we can consider just increasing it if it's a problem.

@bkchr
Copy link
Member

bkchr commented Jul 20, 2022

Yeah, I told them to open an issue, but they don't have done it. However, it should now be enough for them.

DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
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.

4 participants