-
Notifications
You must be signed in to change notification settings - Fork 575
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
module instance context api #2436
Conversation
should be unified with wasm_runtime_set_custom_data? |
@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 |
@@ -394,6 +407,155 @@ wasm_native_unregister_natives(const char *module_name, | |||
return false; | |||
} | |||
|
|||
static uint32 | |||
context_key_to_idx(void *key) |
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.
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.
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.
i can make it a build-time option if desirable.
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.
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.
@@ -394,6 +407,155 @@ wasm_native_unregister_natives(const char *module_name, | |||
return false; | |||
} | |||
|
|||
static uint32 | |||
context_key_to_idx(void *key) | |||
{ |
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.
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?
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.
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.
@@ -939,6 +939,27 @@ WASM_RUNTIME_API_EXTERN bool | |||
wasm_runtime_unregister_natives(const char *module_name, | |||
NativeSymbol *native_symbols); | |||
|
|||
/* See wasm_export.h for description */ | |||
WASM_RUNTIME_API_EXTERN void * | |||
wasm_runtime_module_instance_context_key_create( |
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.
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
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.
Or wasm_runtime_create_module_isnt_ctx_key, wasm_runtime_set_module_inst_ctx?
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.
i agree it's long.
i'm not sure if it's confusing.
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.
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.
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.
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.
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.
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 macroWASM_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.
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.
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
andwasm_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)
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.
Do you mean somewhat like
wasm_runtime_module_inst_create_context_key
andwasm_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 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.
There is no problem if there is an implicit rule that module_instance
is omitted.
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.
It's also OK to me, thanks
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
core/iwasm/common/wasm_native.h
Outdated
@@ -68,6 +68,32 @@ bool | |||
wasm_native_unregister_natives(const char *module_name, | |||
NativeSymbol *native_symbols); | |||
|
|||
#if WASM_ENABLE_MODULE_INST_CONTEXT != 0 | |||
void * | |||
wasm_native_create_context_key(void (*dtor)(wasm_module_inst_t inst, |
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.
Had better use WASMModuleInstanceCommon *
instead of wasm_module_inst_t
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.
what are the principles?
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.
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.
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. i will change.
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.
done
|
||
#if WASM_ENABLE_MODULE_INST_CONTEXT != 0 | ||
void * | ||
wasm_runtime_create_context_key(void (*dtor)(wasm_module_inst_t inst, |
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.
Same as above
@@ -939,6 +939,25 @@ WASM_RUNTIME_API_EXTERN bool | |||
wasm_runtime_unregister_natives(const char *module_name, | |||
NativeSymbol *native_symbols); | |||
|
|||
/* See wasm_export.h for description */ | |||
WASM_RUNTIME_API_EXTERN void * | |||
wasm_runtime_create_context_key(void (*dtor)(wasm_module_inst_t inst, |
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.
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.
#2460
todo:
rename functions:make it a build-time option: