module instance context api #2436

yamt · 2023-08-08T05:09:39Z

introduce module instance context api
make wasi use it
add a sample

todo:

~~rename functions:~~

wasm_runtime_create_context_key
wasm_runtime_destroy_context_key
wasm_runtime_set_context
wasm_runtime_set_context_spread
wasm_runtime_get_context

~~make it a build-time option:~~

WAMR_BUILD_MODULE_INST_CONTEXT
WASM_ENABLE_MODULE_INST_CONTEXT

yamt · 2023-08-08T05:10:29Z

should be unified with wasm_runtime_set_custom_data?

wenyongh · 2023-08-14T06:53:27Z

@yamt The code change is a little big and not sure whether it is good to use the module instance context api, shall we keep this PR unmerged before we release 1.2.3? And could you open an issue to describe module instance context api and why we need to use it? Had better let more people can know that and participate the discussion.

yamt · 2023-08-14T06:56:26Z

@yamt The code change is a little big and not sure whether it is good to use the module instance context api, shall we keep this PR unmerged before we release 1.2.3? And could you open an issue to describe module instance context api and why we need to use it? Had better let more people can know that and participate the discussion.

ok!

yamt · 2023-08-14T07:27:48Z

@yamt The code change is a little big and not sure whether it is good to use the module instance context api, shall we keep this PR unmerged before we release 1.2.3? And could you open an issue to describe module instance context api and why we need to use it? Had better let more people can know that and participate the discussion.

ok!

i created an issue. #2460

core/iwasm/interpreter/wasm_runtime.h

wenyongh · 2023-08-28T02:54:52Z

core/iwasm/common/wasm_native.c

 }

+static uint32
+context_key_to_idx(void *key)


Suggest to make this feature configurable with cmake variable and compiler macro, and it is enabled only when libc wasi is enabled? Or just using cmake -DWAMR_BUILID_LIBC_WASI/UVWASI=1 to control it? Since the feature may introduce more memory usage and binary size, some host embedder may expect the footprint as small as possible.

i can make it a build-time option if desirable.

I'd like to control wasi-nn context with this API for both libc wasi and libc builtin.
I think that the feature can also be helpful for a generic native library.

wenyongh · 2023-08-28T03:07:00Z

core/iwasm/common/wasm_native.c


+static uint32
+context_key_to_idx(void *key)
+{


And not very sure what is the difference from wasm_runtime_set_custom_data, which also binds a custom pointer to the module instance? Is it because that wasm_runtime_module_instance_{set|get}_context can set/get context with key while wasm_runtime_{set|get}_custom_data can not?

Is it because that wasm_runtime_module_instance_{set|get}context can set/get context with key while wasm_runtime{set|get}_custom_data can not?

right.
i want something which can be used by multiple subsystems.

besides that, i want it to have a cleanup callback on instance destory.

wenyongh · 2023-08-28T03:16:29Z

core/iwasm/common/wasm_runtime_common.h


+/* See wasm_export.h for description */
+WASM_RUNTIME_API_EXTERN void *
+wasm_runtime_module_instance_context_key_create(


The API name may be a little long and confusing? How about:

wasm_runtime_create_custom_ctx_key wasm_runtime_destroy_custom_ctx_key wasm_runtime_set_custom_context (or set_custom_ctx) wasm_runtime_set_custom_context_spread wasm_runtime_get_custom_context wasm_native_create_custom_ctx_key wasm_native_destroy_custom_ctx_key wasm_native_set_custom_context (or set_custom_ctx) wasm_native_set_custom_context_spread wasm_native_get_custom_context wasm_native_call_custom_context_dtors

Or wasm_runtime_create_module_isnt_ctx_key, wasm_runtime_set_module_inst_ctx?

i agree it's long.
i'm not sure if it's confusing.

I prefer to keep module_instance for these API names.
It would be easier for me to understand by name whether this API should be used for each module or module_instance.

yes, but adding prefix wasm_runtime really makes me confused, e.g. wasm_runtime_module_instance_set_context, how about:

wasm_module_instance_create_context_key wasm_module_instance_destroy_context_key wasm_module_instance_set_context wasm_module_instance_set_context_spread wasm_module_instance_get_context

And using cmake -DWAMR_BUILD_MODULE_INST_CONTEXT=1/0 and macro WASM_ENABLE_MODULE_INST_CONTEXT to control the feature.

The feature can be enabled no matter whether libc-wasi/libc-builtin is enabled or not.

And when libc-wasi or libc-uvwasi is enabled, the feature will be also enable automatically.

yes, but adding prefix wasm_runtime really makes me confused, e.g. wasm_runtime_module_instance_set_context, how about:

i thought we were prefixing all api with wasm_runtime_.

Do you mean somewhat like wasm_runtime_module_inst_create_context_key and wasm_runtime_module_inst_set_context?

And using cmake -DWAMR_BUILD_MODULE_INST_CONTEXT=1/0 and macro WASM_ENABLE_MODULE_INST_CONTEXT to control the feature.

instance vs INST will make me confused. let's be consistent. which do you prefer?

module_inst in API names and MODULE_INST in cmake variable and compiler macro are better for me, and if you prefer using wasm_runtime_ prefix for API:

wasm_runtime_module_inst_create_context_key wasm_runtime_module_inst_destroy_context_key wasm_runtime_module_inst_set_context wasm_runtime_module_inst_set_context_spread wasm_runtime_module_inst_get_context WAMR_BUILD_MODULE_INST_CONTEXT WASM_ENABLE_MODULE_INST_CONTEXT

Not sure if it is good to others.

yes, but adding prefix wasm_runtime really makes me confused, e.g. wasm_runtime_module_instance_set_context, how about:

i thought we were prefixing all api with wasm_runtime_.

Do you mean somewhat like wasm_runtime_module_inst_create_context_key and wasm_runtime_module_inst_set_context?

as most of the existing apis in wasm_export.h seem to have the wasm_runtime_ prefix,
i thought there was a project-wide policy.
it isn't my own preference.
if you prefer shorter names, it's also ok for me.

also, module_instance/module_inst part is not essential for me either.
after all, many of our api is implicitly about module instances. (eg. wasm_runtime_instantiate, wasm_runtime_set_custom_data, etc)

Do you mean somewhat like wasm_runtime_module_inst_create_context_key and wasm_runtime_module_inst_set_context?

as most of the existing apis in wasm_export.h seem to have the wasm_runtime_ prefix, i thought there was a project-wide policy. it isn't my own preference. if you prefer shorter names, it's also ok for me.

Agree.

also, module_instance/module_inst part is not essential for me either. after all, many of our api is implicitly about module instances. (eg. wasm_runtime_instantiate, wasm_runtime_set_custom_data, etc)

So you mean:

wasm_runtime_create_context_key wasm_runtime_destroy_context_key wasm_runtime_set_context wasm_runtime_set_context_spread wasm_runtime_get_context

It is OK for me, looks concise.

@xujuntwt95329 @tonibofarull What's you opinion? Is it good to you?

There is no problem if there is an implicit rule that module_instance is omitted.

It's also OK to me, thanks

wenyongh

LGTM

wenyongh · 2023-09-05T08:32:42Z

core/iwasm/common/wasm_native.h


+#if WASM_ENABLE_MODULE_INST_CONTEXT != 0
+void *
+wasm_native_create_context_key(void (*dtor)(wasm_module_inst_t inst,


Had better use WASMModuleInstanceCommon * instead of wasm_module_inst_t

what are the principles?

Hi, in the original design, the wasm_module_t/wasm_module_inst_t/wasm_exec_env_t/.. are only used only in wasm_export.h, native libraries (libc-builtin, libc-wasi and more in core/iwasm/libraries) and main.c, and the detailed structure name (WASMModule/WASMModuleInstance/WASMExecEnv/..) are used in runtime internal. Though there may be some code inside runtime using the the formers, we had better obey the rules.

ok. i will change.

wenyongh · 2023-09-05T08:33:26Z

core/iwasm/common/wasm_runtime_common.c

+
+#if WASM_ENABLE_MODULE_INST_CONTEXT != 0
+void *
+wasm_runtime_create_context_key(void (*dtor)(wasm_module_inst_t inst,


Same as above

wenyongh · 2023-09-05T08:33:45Z

core/iwasm/common/wasm_runtime_common.h


+/* See wasm_export.h for description */
+WASM_RUNTIME_API_EXTERN void *
+wasm_runtime_create_context_key(void (*dtor)(wasm_module_inst_t inst,


Same as above

for completeness. now we can implement wasm_runtime_set_custom_data on top of this if we want.

Introduce module instance context APIs which can set one or more contexts created by the embedder for a wasm module instance: ```C wasm_runtime_create_context_key wasm_runtime_destroy_context_key wasm_runtime_set_context wasm_runtime_set_context_spread wasm_runtime_get_context ``` And make libc-wasi use it and set wasi context as the first context bound to the wasm module instance. Also add samples. Refer to bytecodealliance#2460.

yamt mentioned this pull request Aug 14, 2023

module instance context api #2460

Closed

wenyongh reviewed Aug 28, 2023

View reviewed changes

tonibofarull approved these changes Aug 28, 2023

View reviewed changes

yamt force-pushed the inst-context branch from cea823c to 3f8dd63 Compare September 5, 2023 03:08

wenyongh reviewed Sep 5, 2023

View reviewed changes

yamt added 20 commits September 5, 2023 18:35

wip module instance context

01b5138

wip

c72f253

wip

4b99d55

wip

fdb03b0

wip

fd81be0

call wasm_native_module_instance_inherit_contexts where appropriate

2c7554d

call wasm_native_module_instance_call_context_dtors where appropriate

7c4cb73

make wasi to use the new instance context api

bde9f86

export module instance context api

3277234

add definitions to wasm_export.h

3cfc4df

comment

9847807

a copy of basic

b53dfed

a sample app for module instance context api

b6e4dfc

mark wasi_ctx unused

cdd5b18

wasm_module_inst_extra_common: fix aot case

6ed953e

fix WASM_ENABLE_LIBC_WASI check

8f559b7

add wasm_native_module_instance_set_context_spread

856f93e

for completeness. now we can implement wasm_runtime_set_custom_data on top of this if we want.

add a sample for wasm_native_module_instance_set_context_spread

15e3b3d

make WASM_MAX_INSTANCE_CONTEXTS overridable

c59b752

comment

7a1a248

yamt added 8 commits September 5, 2023 18:36

rename module instance context functions

fbe265f

apply clang-format after function renames

e096ad3

add WAMR_BUILD_MODULE_INST_CONTEXT/WASM_ENABLE_MODULE_INST_CONTEXT

9651158

product-mini/platforms/nuttx/wamr.mk: add MODULE_INST_CONTEXT config

46b7644

rename a few internal functions as well

0a8d461

conditionalize code on WASM_ENABLE_MODULE_INST_CONTEXT

763f323

fix wamrc build

2308d23

fix build

3eaa92f

yamt force-pushed the inst-context branch from 305b50a to 3eaa92f Compare September 5, 2023 09:37

use WASMModuleInstanceCommon where appropriate

5ba2a60

wenyongh merged commit 6c846ac into bytecodealliance:main Sep 7, 2023

module instance context api #2436

module instance context api #2436

Uh oh!

Conversation

yamt commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamt commented Aug 8, 2023

Uh oh!

wenyongh commented Aug 14, 2023

Uh oh!

yamt commented Aug 14, 2023

Uh oh!

yamt commented Aug 14, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyongh Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyongh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yamt commented Aug 8, 2023 •

edited

Loading

wenyongh Aug 28, 2023 •

edited

Loading