-
Notifications
You must be signed in to change notification settings - Fork 639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow overwriting max memory on module instantiation #3198
Allow overwriting max memory on module instantiation #3198
Conversation
Does it suggest that the |
Is that not allowed? Actually I remember that was mentioned https://bytecodealliance.zulipchat.com/#narrow/stream/349267-wasi-threads/topic/rust/near/402263578, but couldn't find it in the specs. |
No, from the comment of Alex, the max_memory_pages specified by the API can be smaller than what it is specified in wasm file, but should not be larger than it. So how about we add checks: if max_memory_pages from API is smaller than memory->init_page_count, then set it memory->init_page_count; and if max_memory_pages is larger than memory->max_page_count, then set it to memory->max_page_count? |
Yeah, makes sense, I'll change the code to do that |
9eac4f5
to
affa7a5
Compare
uint32 module_max_page_count) | ||
{ | ||
if (max_memory_pages < module_init_page_count) { | ||
LOG_WARNING("Cannot override max memory with value lower than module " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure is it better to use LOG_WARNING("warning: ...")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user is overriding the logger (i.e. WAMR_BH_LOG
), they probably already have something to distinguish between different log levels: different colors (red for error, yellow for warning) or different log suffixes (e.g. W
, E
suffixes). So adding the log level to the log message is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it, thanks.
affa7a5
to
5282ec8
Compare
5282ec8
to
7e1b36a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
uint32 module_max_page_count) | ||
{ | ||
if (max_memory_pages < module_init_page_count) { | ||
LOG_WARNING("Cannot override max memory with value lower than module " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it, thanks.
…#3198) This PR adds a max_memory_pages parameter to module instantiation APIs, to allow overriding the max memory defined in the WASM module. Sticking to the max memory defined in the module is quite limiting when using shared memory in production. If targeted devices have different memory constraints, many wasm files have to be generated with different max memory values. And device constraints may not be known in advance. Being able to set the max memory value during module instantiation allows to reuse the same wasm module, e.g. by retrying instantiation with different max memory value.
This PR adds a
max_memory_pages
parameter to module instantiation APIs, to allow overriding the max memory defined in the WASM module.Sticking to the max memory defined in the module is quite limiting when using shared memory in production. If targeted devices have different memory constraints, many wasm files have to be generated with different max memory values. And device constraints may not be known in advance.
Being able to set the max memory value during module instantiation allows to reuse the same wasm module, e.g. by retrying instantiation with different max memory value.