Add memory instance support apis #3786
Conversation
|
Just my thoughts, it seems that functions(and code) related to exporting should not be limited by the |
core/iwasm/common/wasm_memory.c
Outdated
| #if WASM_ENABLE_SHARED_MEMORY != 0 | ||
| shared_memory_lock(memory); | ||
| #endif | ||
| ret = wasm_enlarge_memory_internal(NULL, memory, (uint32)inc_page_count); |
There was a problem hiding this comment.
Could we add argument module for wasm_memory_inst_enlarge and here pass it to wasm_enlarge_memory_internal? Since module may be used in wasm_enlarge_memory_internal, and developer should know it when calling wasm_memory_inst_enlarge.
bool
wasm_memory_inst_enlarge(WASMModuleInstanceCommon *module_inst,
WASMMemoryInstance *memory, uint64 inc_page_count)
{
...
wasm_enlarge_memory_internal(module_inst, memory, ..);
...
}
There was a problem hiding this comment.
@wenyongh in order to support externally created (imported) memory objects, you have to be able to create them independent of a module or module instance, so I think we should leave this API without a required module argument. However, I'm OK with adding a module argument and making it optional.
| @@ -913,21 +908,20 @@ wasm_enlarge_memory_internal(WASMModuleInstance *module, uint32 inc_page_count, | |||
| wasm_runtime_set_mem_bound_check_bytes(memory, total_size_new); | |||
|
|
|||
| return_func: | |||
| if (!ret && enlarge_memory_error_cb) { | |||
| if (!ret && module && enlarge_memory_error_cb) { | |||
There was a problem hiding this comment.
If we add argument module to wasm_memory_inst_enlarge and passing argument module to wasm_enlarge_memory_internal, then here we don't add the check && module.
There was a problem hiding this comment.
This relates to the previous comments, if we added a module argument to wasm_memory_inst_enlarge(), I think it should be optional. If so, we would still need to check it here.
core/iwasm/include/wasm_export.h
Outdated
| */ | ||
| WASM_RUNTIME_API_EXTERN bool | ||
| wasm_memory_inst_enlarge(wasm_memory_inst_t memory_inst, | ||
| uint64_t inc_page_count); |
There was a problem hiding this comment.
There is already a function bool wasm_runtime_enlarge_memory(module_inst, inc_page_count) but it enlarge the default memory. Do you think it is better to use wasm_runtime_enlarge_memory_with_idx?
There was a problem hiding this comment.
This relates to previous comments:
#3786 (comment)
#3786 (comment)
For memories created independently of a module or module instance, providing an index has no meaning, so I think this API should remain like this.
Now that WAMR supports multiple memory instances, this adds some APIs to access them in a standard way.
This involved moving some existing utility functions out from the
WASM_ENABLE_MULTI_MODULEblocks they were nested in, but multi-memory and multi-module seem independent as far as I can tell so I assume that's okay.