Enable concurrent instantiation, and release #342
Conversation
|
ok cool no race detected |
| // This case other modules are importing this module instance and still alive. | ||
| return fmt.Errorf("%d modules import this and need to be closed first", instance.refCount) | ||
| return fmt.Errorf("%d modules import this and need to be closed first", instance.importedCount) | ||
| } | ||
|
|
||
| // TODO: check outstanding calls and wait until they exit. |
There was a problem hiding this comment.
this will be resolved in the follow-up and then we could expose Close (or whatever we call) in the public API
mathetake
changed the title
mathetake
changed the title
|
I'm going to add some commits instead of making review comments (or at least to reduce them) |
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
There was a problem hiding this comment.
one main question on the impl, otherwise works for me!
PS I think in the future I'd also like to understand why the store can't partition data by module as it seems the locks would be less blinding. I guess there are store-wide type indexes that must be coherent across modules? If this is already explained in RATIONALE never mind, but one day if it isn't let's add something on the data partitioning strategy as it impacts locking.
| // mux is used to guard the fields from concurrent access. | ||
| mux sync.Mutex | ||
|
|
||
| // dependentCount is the current number of modules which import this module. On Store.ReleaseModule, this number |
There was a problem hiding this comment.
notably renamed these two fields
Signed-off-by: Adrian Cole <[email protected]>
Theoretically can be partitioned, but a live instance must be assigned a unique addr (funcaddr, memaddr, etc in the spec https://www.w3.org/TR/wasm-core-1/#addresses%E2%91%A0), and notably the function calls were made via funcaddr (which engine maps to the a *compiledFunction). In the spec rather aggressively, a module instance only holds addresses (https://www.w3.org/TR/wasm-core-1/#module-instances%E2%91%A0). But yeah I believe there's some way to work around this (especially once we are able to do precompilation namely after we could decouple compiledFunction from runtime representations). |
Signed-off-by: Takeshi Yoneda <[email protected]>
|
Raise a follow-up for the ps comment! #343 |
|
thanks for the work on this, getting close to safe Close! |
This commit adds ModuleEngine interface which is used to make calls and created per-module instance. Notably, this enables us to remove the necessity for store to holds FunctionInstances and FunctionIndex. This is a follow-up from #342 Signed-off-by: Takeshi Yoneda <[email protected]> Co-authored-by: Adrian Cole <[email protected]>
This makes sure that concurrent use of Instantiate and
ReleaseModuleInstance is goroutine-safe.
Note that it is still not safe to expose Release in the public API
as we haven't taken into account the outstanding calls. That
would be addressed in the follow-up commit.
part of #293.
Signed-off-by: Takeshi Yoneda [email protected]