Skip to content

wasi-nn: add minimum serialization on WASINNContext #4387

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

Merged
merged 1 commit into from
Jun 20, 2025

Conversation

yamt
Copy link
Collaborator

@yamt yamt commented Jun 18, 2025

currently this is not necessary because context (WASINNContext) is local to instance. (wasm_module_instance_t)

i plan to make a context shared among instances in a cluster when fixing #4313. this is a preparation for that direction.

an obvious alternative is to tweak the module instance context APIs to allow declaring some kind of contexts instance-local. but i feel, in this particular case, it's more natural to make "wasi-nn handles" shared among threads within a "process".

note that, spec-wise, how wasi-nn behaves wrt threads is not defined at all because wasi officially doesn't have threads yet. i suppose, at this point, that how wasi-nn interacts with wasi-threads is something we need to define by ourselves, especially when we are using an outdated wasi-nn version.

with this change, if a thread attempts to access a context while another thread is using it, we simply make the operation fail with the "busy" error. this is intended for the mimimum serialization to avoid problems like crashes/leaks/etc. this is not intended to allow parallelism or such.

no functional changes are intended at this point yet.

cf.
#4313
#2430

currently this is not necessary because context (WASINNContext) is
local to instance. (wasm_module_instance_t)

i plan to make a context shared among instances in a cluster when
fixing bytecodealliance#4313.
this is a preparation for that direction.

an obvious alternative is to tweak the module instance context APIs
to allow declaring some kind of contexts instance-local. but i feel,
in this particular case, it's more natural to make "wasi-nn handles"
shared among threads within a "process".

note that, spec-wise, how wasi-nn behaves wrt threads is not defined
at all because wasi officially doesn't have threads yet. i suppose, at
this point, that how wasi-nn interacts with wasi-threads is something
we need to define by ourselves, especially when we are using an outdated
wasi-nn version.

with this change, if a thread attempts to access a context while
another thread is using it, we simply make the operation fail with
the "busy" error. this is intended for the mimimum serialization to
avoid problems like crashes/leaks/etc. this is not intended to allow
parallelism or such.

no functional changes are intended at this point yet.

cf.
bytecodealliance#4313
bytecodealliance#2430
Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saying, I used to think you would prefer to use atomic variables to achieve similar functionality.

@lum1n0us lum1n0us merged commit ea408ab into bytecodealliance:main Jun 20, 2025
426 checks passed
@yamt
Copy link
Collaborator Author

yamt commented Jun 20, 2025

Just saying, I used to think you would prefer to use atomic variables to achieve similar functionality.

originally i wanted to use trylock. (and filed #4386)

direct uses of atomics would make it a bit difficult to read/maintain. it can be done later if/when it turns out desirable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants