[instantiation linking] Create and import WASMFunctionInstance #3947
Conversation
lum1n0us
force-pushed
the
feat/inst_linking_function
branch
from
68f35cb to
261f2a8
Compare
lum1n0us
requested review from
loganek,
no1wudi,
wenyongh,
xujuntwt95329 and
yamt
as code owners
…pawned' for clarity in deinstantiation functions
…ecks and improving error handling
…nagement functions
…conv_wasm_c_api and call_conv_raw in WASM function structures
… for function instantiation
…pport and improve error logging
…pport and improve error handling
… support and improve clarity
…structure and improving function import handling
… support and clarity
…prove function instance lookup
lum1n0us
force-pushed
the
feat/inst_linking_function
branch
from
1c78639 to
cbb33b4
Compare
|
@wenyongh Reviewing this extensive document may be time-consuming, but I still require your input to begin the subsequent optimization process. ☕ 🍺 |
| set_error_buf_v(error_buf, error_buf_size, | ||
| "unknown import module \"%s\"", | ||
| import->module_name); | ||
| return NULL; |
There was a problem hiding this comment.
Should destroy functions before return, same as the below return NULL lines.
| import_func->module_name, import_func->func_name); | ||
|
|
||
| *func_ptrs = import_func->func_ptr_linked; | ||
| bh_assert(*func_ptrs); |
There was a problem hiding this comment.
The import func ptr is not "must be prepared", the original code in aot loader reports "warning: failed to link import function", and when execution, runtime throws "failed to call unlinked import function". We had better not change the behavior?
There was a problem hiding this comment.
If my memory serves me right, the initial design was due to AOT not fully supporting the multi-module feature until it was contributed by the community. However, it wasn't a complete implementation and had some gaps, such as the one we're discussing.
In this round, I'm inclined to align with the specification requirements and maintain the same expected behavior as the wasm_runtime. Like: all functions, both imported and local ones should be set up prior to execution..
| uint64 total_size = sizeof(AOTFunctionInstance) * (uint64)function_count; | ||
|
|
||
| AOTFunctionInstance *functions = | ||
| runtime_malloc(total_size, error_buf, error_buf_size); |
There was a problem hiding this comment.
In the original implementation, we first only create function instances of export functions instead all the instances. The latter are only created in special and rare situation, e.g., called from wasm_table_get_func_inst. It may increase the memory consumption a lot, e.g., there are lots of wasm functions but only several functions are exported, I think we had better not change the optimization, or only create import/export func instances at first, or discuss with others?
There was a problem hiding this comment.
only create import/export func instances
It seems feasible. Let me start working on it.
| /* write it from wasm_c_api */ | ||
| bool call_conv_wasm_c_api; | ||
| /* AOTModuleInstance collects it to form c_api_func_imports */ | ||
| CApiFuncImport import_func_c_api; |
There was a problem hiding this comment.
These fields are only required by import functions, adding them here will increase memory consumption when non-import function instances are created, could we get them or most of them from the AOTImportFunc *func_import?
There was a problem hiding this comment.
It's quite challenging to do that.
AOTModuleInstance *import_module_inst and struct AOTFunctionInstance *import_func_inst are used for instantiation linking with other modules, so we need to retain both here.
bool call_conv_raw is utilized for registered native symbols and can be taken from AOTImportFunc.
bool call_conv_wasm_c_api and CApiFuncImport import_func_c_api are created by the wasm_c_api APIs. Both should be capable of instantiation linking.
Therefore, only one bool field is duplicated from AOTImportFunc. The other fields need to remain in AOTFunctionInstance to meet the requirements of instantiation linking.
No description provided.