Call main only once #3092
Call main only once #3092
Conversation
daxpedda
force-pushed
the
main-only-once
branch
2 times, most recently
from
c097e46
to
ac6cf1f
Compare
daxpedda
force-pushed
the
main-only-once
branch
from
ac6cf1f
to
e5cc853
Compare
|
@Liamolucko this is ready now. |
| // This happens after the thread counter was already increased, so "1" | ||
| // means we are in the first thread. | ||
| body.i32_const(self.0) | ||
| .load(memory, LoadKind::I32 { atomic: true }, ATOMIC_MEM_ARG) |
There was a problem hiding this comment.
I think this could lead to a race condition - if a second thread starts between when the counter is incremented from 0 to 1 and when we load it here, this could be read as 2 instead of 1 despite this being the first thread, and then the start function won't get run at all.
This'll have to somehow get access to the result of the initial fetch-add done in inject_start to avoid the race condition.
There was a problem hiding this comment.
So the reason why I thought this can not become a race condition is because this is called as part of the init function and you can't start a second thread because you don't have access to the WASM instantiation until the init function is finished.
I don't consider this brittle either because changing this behavior would be a really big change.
Still, I consider your suggestions an improvement, I will look into it now.
There was a problem hiding this comment.
So I found two ways to potentially solve this:
- The injected thread shim returns the thread counter, which we can take later and work with. The problem is that if we change the order of when shims are injected or if we insert a new shim, this will break, and I'm not sure how to fix that except making things more complicated and pass the thread counter around.
- Save the thread counter into a thread local variable. Unfortunately I don't know how to make a thread local variable.
I consider the first solution worse then the current one, so my preference is to keep the current solution as I'm fairly certain it is correct.
There was a problem hiding this comment.
I went through my assumptions again, I'm certain that this can't become a race condition.
Simply put, to make this a race condition the init function has to return the instantiated module without calling the start (__wbindgen_start) function, which would be a breaking change.
There was a problem hiding this comment.
init isn't the only way to get a compiled module, though - you can use WebAssembly.compile to compile the module in advance, create a SharedArrayBuffer, and pass them to two threads which call init at the same time.
(I'm not sure what you mean by an 'instantiated module', but init takes a compiled WebAssembly.Module when initialising another thread.)
There was a problem hiding this comment.
initisn't the only way to get a compiled module, though - you can useWebAssembly.compileto compile the module in advance, create aSharedArrayBuffer, and pass them to two threads which callinitat the same time.
I thought that this isn't supported? For example I took it that your comment here states that: #3224 (comment).
I guess if this is supported, then I have to implement the solution lined out above.
(I'm not sure what you mean by an 'instantiated module', but
inittakes a compiledWebAssembly.Modulewhen initialising another thread.)
Yes, that is what I mean. You can't get the WebAssembly.Module without calling the init function, unless, like you pointed out, you don't use the init function and compile it yourself.
There was a problem hiding this comment.
See #3234, which addresses this issue.
|
Replaced by #3234. |
As discussed in #3062, we use an atomic to only call the main function once.
For context, this change adjusts the
initfunction to only callmainor the function exported by# [wasm_bindgen(start)]only once even ifinitis called multiple times. This is important when using it in a context of spawning workers, otherwise each worker would call themainfunction again, which is probably unintended.This might be considered a breaking change, because somebody out there might be relying on this behavior. But considering this is a nightly only feature (I believe), it should be fine?
The way I did this here is to re-use the thread counter. I had to somehow return it from
wasm_bindgen_threads_xform::Config::run(), where it is created, so I created aThreadCounterAddrthat also has a method that is able to wrap the main function behind the atomic check.I'm not really familiar with the code-base, but I did try my best to implement this as cleanly as possible, I'm open to suggestions of course.
Replaces #3062.