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

Add extra WASM heap pages when precompiling the runtime blob #11107

Merged
merged 7 commits into from
Mar 24, 2022
Merged

Add extra WASM heap pages when precompiling the runtime blob #11107

merged 7 commits into from
Mar 24, 2022

Conversation

koute
Copy link
Contributor

@koute koute commented Mar 24, 2022 •
edited
Loading

@koute koute added A0-please_review Pull request needs code review. I3-bug The node fails to follow expected behavior. I5-tests Tests need fixing, improving or augmenting. 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 Mar 24, 2022
@koute koute requested review from pepyakin and a team March 24, 2022 07:21
@koute koute mentioned this pull request Mar 24, 2022
Merged
wigy-opensource-developer
wigy-opensource-developer approved these changes Mar 24, 2022
View reviewed changes
Copy link
Contributor

@wigy-opensource-developer wigy-opensource-developer left a comment

Choose a reason for hiding this comment

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

Besides the rustdoc, LGTM

client/executor/wasmtime/src/runtime.rs Outdated Show resolved Hide resolved
client/executor/wasmtime/src/tests.rs Outdated Show resolved Hide resolved
@wigy-opensource-developer
Copy link
Contributor

I was too fast. You fixed the rustdoc while I was reviewing 😉

bkchr
bkchr suggested changes Mar 24, 2022
View reviewed changes
Copy link
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.

I think this is incorrect. Maybe it was already incorrect before, but the heap pages should be the maximum. So, if polkadot it setting these for parachains it is 1gb and nothing in the wasm file should allow to exceed this value. Currently the wasm file could specify whatever initial value it wants and it would get this plus the heap pages we are requesting by polkadot?

For normal runtimes this isn't such a big problem, as they are not provided by the user.

@koute
Copy link
Contributor Author

koute commented Mar 24, 2022

I think this is incorrect. Maybe it was already incorrect before, but the heap pages should be the maximum.

This is indeed how it worked originally, I just changed the name of the field to actually say what it does.

We can consider changing it, and I agree with you that changing this would make sense (which is what I wrote in this comment), but I think that'd be better for another PR as this just restores the original behavior from before the #10480 landed.

So, if polkadot it setting these for parachains it is 1gb and nothing in the wasm file should allow to exceed this value. Currently the wasm file could specify whatever initial value it wants and it would get this plus the heap pages we are requesting by polkadot?

For normal runtimes this isn't such a big problem, as they are not provided by the user.

Yes, the blob can request whatever value it wants, and it will get that amount of heap pages plus the extra_heap_pages. But there's still an absolute limit set by the Config::max_memory_size. If it goes over that then the instantiation will fail, so in practice this is - while weird - not a problem. (For PVFs the limit is set here, so with the current limit a PVF can get at most 2080 pages allocated, which is ~130MB of memory.)

bkchr
bkchr approved these changes Mar 24, 2022
View reviewed changes
Copy link
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.

Okay, sounds good.

@bkchr bkchr merged commit dfb2a8c into paritytech:master Mar 24, 2022
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@koute @grishasobol
Add extra WASM heap pages when precompiling the runtime blob (parityt…
2a30188 
…ech#11107)

* Add extra WASM heap pages when precompiling the runtime blob

* Fix compilation

* Fix rustdoc

* Fix rustdoc for real this time

* Fix benches compilation

* Improve the builder in `sc-executor-wasmtime`'s tests
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@koute @grishasobol
Add extra WASM heap pages when precompiling the runtime blob (parityt…
d9e1c0a 
…ech#11107)

* Add extra WASM heap pages when precompiling the runtime blob

* Fix compilation

* Fix rustdoc

* Fix rustdoc for real this time

* Fix benches compilation

* Improve the builder in `sc-executor-wasmtime`'s tests
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@koute @grishasobol
Add extra WASM heap pages when precompiling the runtime blob (parityt…
b76b81c 
…ech#11107)

* Add extra WASM heap pages when precompiling the runtime blob

* Fix compilation

* Fix rustdoc

* Fix rustdoc for real this time

* Fix benches compilation

* Improve the builder in `sc-executor-wasmtime`'s tests
@github-actions github-actions bot mentioned this pull request Apr 20, 2022
Closed
@github-actions github-actions bot mentioned this pull request Jul 18, 2022
Open
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@koute @DaviRain-Su
Add extra WASM heap pages when precompiling the runtime blob (parityt…
1252e4d 
…ech#11107)

* Add extra WASM heap pages when precompiling the runtime blob

* Fix compilation

* Fix rustdoc

* Fix rustdoc for real this time

* Fix benches compilation

* Improve the builder in `sc-executor-wasmtime`'s tests
@koute koute mentioned this pull request Sep 29, 2022
Merged
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@koute @ark0f
Add extra WASM heap pages when precompiling the runtime blob (parityt…
76a975f 
…ech#11107)

* Add extra WASM heap pages when precompiling the runtime blob

* Fix compilation

* Fix rustdoc

* Fix rustdoc for real this time

* Fix benches compilation

* Improve the builder in `sc-executor-wasmtime`'s tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wigy-opensource-developer wigy-opensource-developer wigy-opensource-developer approved these changes

@KiChjang KiChjang KiChjang approved these changes

@bkchr bkchr bkchr approved these changes

@pepyakin pepyakin Awaiting requested review from pepyakin

Assignees
No one assigned
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 I3-bug The node fails to follow expected behavior. I5-tests Tests need fixing, improving or augmenting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test terminates_on_timeout fails with latest Substrate
4 participants
@koute @wigy-opensource-developer @KiChjang @bkchr