[instantiation linking] Optimize aot func inst list #4018

lum1n0us · 2025-01-13T06:23:56Z

#3947 (comment)

…pawned' for clarity in deinstantiation functions

…r clarity

…or handling

…ontext creation

…ecks and improving error handling

…ction instances

…nagement functions

…conv_wasm_c_api and call_conv_raw in WASM function structures

… for function instantiation

… spec tests

…pport and improve error logging

…pport and improve error handling

… support and improve clarity

… linking logic

…structure and improving function import handling

… support and clarity

…prove function instance lookup

… AOT loader

…T runtime

…ction handling

wenyongh · 2025-01-14T01:16:10Z

core/iwasm/aot/aot_runtime.c

+                                             WASM_IMPORT_EXPORT_KIND_FUNC, i);
+        if (!extern_inst) {
+            LOG_DEBUG("no import function(%s, %s) from imports list, might "
+                      "provied by wasm_native",


wenyongh · 2025-01-14T01:20:04Z

core/iwasm/aot/aot_runtime.c

-            const char *module_name = module->import_funcs[i].module_name;
-            const char *field_name = module->import_funcs[i].func_name;
-            LOG_WARNING("warning: failed to link import function (%s, %s)",
-                        module_name, field_name);


This is an intentional warning, it may be important to developer, could we keep it in the new code if link failed?

wenyongh · 2025-01-14T01:23:11Z

core/iwasm/aot/aot_runtime.c

+                  import_func->module_name, import_func->func_name);
+
+        *func_ptrs = import_func->func_ptr_linked;
+        bh_assert(*func_ptrs);


Per my understanding, the import function doesn't have to be linked and runtime can print warning here, and during execution, the aot code or runtime throws exception "failed to call unlinked import function". The original behavior is like that, we had better not change it?

Regarding that matter, I am inclined to align with the specification requirements, specifically Step #3 and Step #4. If I understand correctly, the linking error should be reported during the installation phase rather than at execution time.

Our current implementation deviates from this, and I intend to correct it.

In this PR, the wasm_runtime implementation is also incorrect and needs to be addressed. I plan to fix both issues in subsequent pull requests.

That brings convenience to developers and is a common handle in many VMs like jvm. In many cases the unlinked import function isn't called and developer can still call other functions. No developer reports it a problem, I strongly suggest to keep this. If you want to fix this, had better open another PR to let others to review/discuss and not fix it in this PR.

In many cases the unlinked import function isn't called and developer can still call other functions.

However, this does not align with the specification definition. I believed the purpose of creating the "instantiation linking" series of pull requests was to align with the specification requirements. Otherwise, our main branch code is functioning quite well.

wenyongh · 2025-01-14T01:25:37Z

core/iwasm/aot/aot_runtime.c

+    extra->function_count = module->import_func_count + module->func_count;
+    if (extra->function_count > 0) {
+        extra->import_functions =
+            functions_instantiate(module_inst, module, imports, import_count,


how about renaming it to import_functions_instantiate?

wenyongh · 2025-01-14T01:35:48Z

core/iwasm/common/wasm_extern_inst.c

If this file isn't needed, had better remove it?

wenyongh · 2025-01-14T01:35:48Z

core/iwasm/common/wasm_extern_inst.h

wenyongh · 2025-01-14T01:35:59Z

core/iwasm/common/wasm_c_api.c

+    }
+
+    *imports_dst =
+        malloc_internal(sizeof(WASMExternInstance) * imports_src->num_elems);


how about (uint64)sizeof(WASMExternInstance)?

wenyongh · 2025-01-14T01:36:07Z

core/iwasm/aot/aot_runtime.h

@@ -225,7 +238,7 @@ typedef struct AOTModule {

    /* function info */
    uint32 func_count;
-    /* func pointers of AOTed (un-imported) functions */
+    /* func pointers of AOTed (imported + local) functions */


how about also changing comment of L243?

func_type_indexes still only stores local(un-imported) functions.

wenyongh · 2025-01-14T01:36:12Z

core/iwasm/aot/aot_runtime.c

+{
+    AOTFunctionInstance *function = runtime_malloc(
+        sizeof(AOTFunctionInstance) + sizeof(AOTImportFunc), NULL, 0);


A little confused why need to plus sizeof(AOTImportFunc)?

I am considering that this would enable host APIs (defined in wasm_export.h) to create an AOTFunctionInstance from scratch and pass it to the runtime for execution. The runtime would then use the u.func_import field to retrieve some information. However, there might be room for optimization in subsequent pull requests.

wenyongh · 2025-01-15T03:22:36Z

core/iwasm/interpreter/wasm_runtime.c

+wasm_create_function_empty(const WASMModule *module)
+{
+    WASMFunctionInstance *function = runtime_malloc(
+        sizeof(WASMFunctionInstance) + sizeof(WASMFunctionImport), NULL, 0);


Is it necessary to allocate extra memory (+ sizeof(WASMFunctionImport))?

wenyongh · 2025-01-15T06:35:58Z

core/iwasm/aot/aot_runtime.c

+                  import_func->module_name, import_func->func_name);
+
+        *func_ptrs = import_func->func_ptr_linked;
+        bh_assert(*func_ptrs);


That brings convenience to developers and is a common handle in many VMs like jvm. In many cases the unlinked import function isn't called and developer can still call other functions. No developer reports it a problem, I strongly suggest to keep this. If you want to fix this, had better open another PR to let others to review/discuss and not fix it in this PR.

lum1n0us · 2025-01-15T07:10:29Z

In many cases the unlinked import function isn't called and developer can still call other functions.

However, this does not align with the specification definition. I believed the purpose of creating the "instantiation linking" series of pull requests was to align with the specification requirements. Otherwise, our main branch code is functioning quite well.

wenyongh · 2025-01-15T07:24:12Z

In many cases the unlinked import function isn't called and developer can still call other functions.

However, this does not align with the specification definition. I believed the purpose of creating the "instantiation linking" series of pull requests was to align with the specification requirements. Otherwise, our main branch code is functioning quite well.

It reports the error during the execution time but not instantiation time, it doesn't strictly obey the spec. But sometimes the user experience is more important, in fact, we also do some special handlings in other places. I still think it is good to keep it, it also reports error and it is more friendly to developers.

…stency

…nce memory allocation consistency

… functions

lum1n0us added 23 commits December 22, 2024 02:33

[inst-link function]Refactor: rename 'is_sub_inst' parameter to 'is_s…

ee41140

…pawned' for clarity in deinstantiation functions

Refactor: simplify function pointer retrieval in aot_call_function fo…

ba233d0

…r clarity

Refactor: streamline import function resolution logic and improve err…

295cbbb

…or handling

Refactor: improve comments for clarity in AOT function handling and c…

8368647

…ontext creation

Refactor: enhance import function resolution by adding module name ch…

d440f6f

…ecks and improving error handling

Refactor: update TODO comment to indicate removal of import_func_ptrs

949eb16

Refactor: add internal functions for creating and destroying WASM fun…

ed63721

…ction instances

Refactor: update WASM function import structure and improve memory ma…

3adfbb9

…nagement functions

Refactor: streamline function call conventions by consolidating call_…

0b25433

…conv_wasm_c_api and call_conv_raw in WASM function structures

Refactor: rename type variables for clarity and improve error logging…

264c245

… for function instantiation

Refactor: add print wrappers and register native functions for spec test

44c0475

Refactor: enhance import function handling and add print wrappers for…

dff5399

… spec tests

Refactor: enhance function pointer initialization for multi-module su…

c9ede66

…pport and improve error logging

Refactor: enhance function pointer initialization for multi-module su…

f9383ce

…pport and improve error handling

Refactor: enhance AOT function and import structures for multi-module…

69d006a

… support and improve clarity

Refactor: enhance C API function import handling and improve function…

e4729df

… linking logic

Refactor: enhance multi-module support by adding sub-module instance …

35949fd

…structure and improving function import handling

Refactor: update function instance handling for improved multi-module…

a4019a3

… support and clarity

Refactor: remove redundant import function pointer assignments and im…

d619dc9

…prove function instance lookup

Refactor: update memory flag handling and improve type consistency in…

1221057

… AOT loader

Refactor: improve function pointer retrieval and error handling in AO…

a68d9f9

…T runtime

Refactor: enhance C API function import handling in native invocation

cbb33b4

Refactor: rename function pointers for clarity and improve import fun…

dbe689a

…ction handling

lum1n0us requested review from loganek, no1wudi, wenyongh, xujuntwt95329 and yamt as code owners January 13, 2025 06:23

lum1n0us mentioned this pull request Jan 13, 2025

[instantiation linking] Create and import WASMFunctionInstance #3947

Open

wenyongh reviewed Jan 14, 2025

View reviewed changes

wenyongh reviewed Jan 15, 2025

View reviewed changes

lum1n0us added 3 commits January 15, 2025 21:38

Refactor: rename function instantiation methods for clarity and consi…

897ca2e

…stency

Refactor: improve error handling for import function linking and enha…

32dab97

…nce memory allocation consistency

Refactor: improve formatting and readability in AOT runtime and C API…

ee34499

… functions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[instantiation linking] Optimize aot func inst list #4018

[instantiation linking] Optimize aot func inst list #4018

lum1n0us commented Jan 13, 2025

wenyongh Jan 14, 2025

wenyongh Jan 14, 2025 •

edited

Loading

wenyongh Jan 14, 2025

lum1n0us Jan 15, 2025

wenyongh Jan 15, 2025

lum1n0us Jan 15, 2025

wenyongh Jan 14, 2025

wenyongh Jan 14, 2025

wenyongh Jan 14, 2025

wenyongh Jan 14, 2025

wenyongh Jan 14, 2025

lum1n0us Jan 15, 2025 •

edited

Loading

wenyongh Jan 14, 2025

lum1n0us Jan 15, 2025

wenyongh Jan 15, 2025

wenyongh Jan 15, 2025

lum1n0us commented Jan 15, 2025

wenyongh commented Jan 15, 2025

[instantiation linking] Optimize aot func inst list #4018

Are you sure you want to change the base?

[instantiation linking] Optimize aot func inst list #4018

Conversation

lum1n0us commented Jan 13, 2025

Choose a reason for hiding this comment

wenyongh Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lum1n0us Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lum1n0us commented Jan 15, 2025

wenyongh commented Jan 15, 2025

wenyongh Jan 14, 2025 •

edited

Loading

lum1n0us Jan 15, 2025 •

edited

Loading