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

Change MaxMemorySize to MaxMemoryPages #6728

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Feb 16, 2023

We should set the max memory for the executor in pages (64KiB) and not in bytes. The wasm memory is always a multiple of a page and we should use the same terminology.

Sorry to not have brought up this earlier 🙈 I just realized while doing some other things 🙈

We should set the max memory for the executor in pages (64KiB) and not in bytes.
The wasm memory is always a multiple of a page and we should use the
same terminology.
@bkchr bkchr 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. A0-pleasereview labels Feb 16, 2023
@s0me0ne-unkn0wn
Copy link
Contributor

I don't have a strong opinion if it's beneficial or not. Substrate executor deals with paging issues itself where needed, and external Wastime interfaces often use bytes to represent different types of limits (e.g. StoreLimitsBuilder::memory_size() and memory_guaranteed_dense_image_size()).

I mean, ExecutorParams are intended to expose executor semantics to the chain. If we want to operate pages, why not change executor semantics itself then (along with changing it here)? That would also somewhat simplify code on the Substrate executor side.

@koute any thoughts from you?

Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

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

LGTM

Even if some of wasmtime's interfaces take a number of bytes instead of a number of pages (for convenience's sake I suppose) it doesn't change the fact that WASM memory can only grow in page increments, so it makes sense to define the limit also in page increments.

Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

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

I'd anyway consider changing that on the Substrate executor side for the sake of consistency, otherwise looks good.

@bkchr
Copy link
Member Author

bkchr commented Feb 16, 2023

I'd anyway consider changing that on the Substrate executor side for the sake of consistency, otherwise looks good.

The change is already coming here: #6730 This is the reason I have opened this pr :D

@bkchr bkchr merged commit 174816c into master Feb 16, 2023
@bkchr bkchr deleted the bkchr-rename-max-memory-to-max-memory-pages branch February 16, 2023 16:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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