Skip to content
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.

Sign up for GitHub

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

Jump to bottom

The wasmtime::Store type should be thread-safe #777

Closed
alexcrichton opened this issue Jan 8, 2020 · 8 comments · Fixed by #1624
Closed

The wasmtime::Store type should be thread-safe #777

alexcrichton opened this issue Jan 8, 2020 · 8 comments · Fixed by #1624
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Jan 8, 2020 •
edited
Loading

As proposed in #708 the Store type in the wasmtime API should be thread-safe, meaning it should implement both Send and Sync. Currently, however, it has a few items that are not thread safe and/or need audits:

  • The global_exports field is an Rc-protected data structure used by wasmtime internals. I don't know myself what it would take to remove this, but @sunfishcode looks like he does in Replace global-exports mechanism with caller-vmctx mechanism #390
  • The signature_cache field requires interior mutability and currently uses a RefCell. While this could be switched to a Mutex I think it would be best to avoid this altogether. I don't personally know why this field exists, @yurydelendik do you know why it's here?
  • The context field has a contained compiler field which has a number of issues. Namely Rc and RefCell are not thread safe (but can be swapped for Arc/Mutex if necessary), but the underlying Compiler type has a number of trait objects, raw pointers, etc, which would need auditing. I think the solution here is going to be making Compiler itself threadsafe by redesigning it's internals.
@yurydelendik
Copy link
Contributor

signature_cache shall be a common storage across all imports/exports/module that are interact between each other to maintain common database of SignatureId -> Signature. The Compiler type contains signatures field to do the reverse operation, which is needed for codegen.

@alexcrichton alexcrichton mentioned this issue Jan 13, 2020
Closed
27 tasks
@alexcrichton alexcrichton added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 13, 2020
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Jan 22, 2020
@alexcrichton
Remove Store::signature_cache field
e24a5d7 
This commit removes the `signature_cache` field from the `Store` type
and performs a few internal changes which are aimed to be a bit forward
looking towards bytecodealliance#777, making `Store` threadsafe.

The changes made here are:

* The `SignatureRegistry` internal type now contains the reverse map
  that `signature_cache` was serving to do. This is populated on calls
  to `register` automatically and is accompanied by a `lookup` method as
  well.

* The `register_wasmtime_signature` and `lookup_wasmtime_signature`
  methods were removed from `Store` and now instead work by using the
  `Compiler::signatures` field.

* The `SignatureRegistry` type was updated to have interior mutability.
  The global `Compiler` type is highly likely to get shared across many
  threads through `Store`, so it needs some form of lock somewhere for
  mutation of the registry of signatures and this commit opts to put it
  inside `SignatureRegistry` which will eventually allow for the removal
  of most `&mut self` method on `Compiler`.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Jan 22, 2020
@alexcrichton
Move the Store::signature_cache field
25dcccc 
This commit removes the `signature_cache` field from the `Store` type
and performs a few internal changes which are aimed to be a bit forward
looking towards bytecodealliance#777, making `Store` threadsafe.

The changes made here are:

* The `SignatureRegistry` internal type now contains the reverse map
  that `signature_cache` was serving to do. This is populated on calls
  to `register` automatically and is accompanied by a `lookup` method as
  well.

* The `register_wasmtime_signature` and `lookup_wasmtime_signature`
  methods were removed from `Store` and now instead work by using the
  `Compiler::signatures` field.

* The `SignatureRegistry` type was updated to have interior mutability.
  The global `Compiler` type is highly likely to get shared across many
  threads through `Store`, so it needs some form of lock somewhere for
  mutation of the registry of signatures and this commit opts to put it
  inside `SignatureRegistry` which will eventually allow for the removal
  of most `&mut self` method on `Compiler`.
@alexcrichton alexcrichton mentioned this issue Jan 22, 2020
Merged
alexcrichton added a commit that referenced this issue Jan 22, 2020
@alexcrichton
Move the Store::signature_cache field (#847)
e5af0ae 
This commit removes the `signature_cache` field from the `Store` type
and performs a few internal changes which are aimed to be a bit forward
looking towards #777, making `Store` threadsafe.

The changes made here are:

* The `SignatureRegistry` internal type now contains the reverse map
  that `signature_cache` was serving to do. This is populated on calls
  to `register` automatically and is accompanied by a `lookup` method as
  well.

* The `register_wasmtime_signature` and `lookup_wasmtime_signature`
  methods were removed from `Store` and now instead work by using the
  `Compiler::signatures` field.

* The `SignatureRegistry` type was updated to have interior mutability.
  The global `Compiler` type is highly likely to get shared across many
  threads through `Store`, so it needs some form of lock somewhere for
  mutation of the registry of signatures and this commit opts to put it
  inside `SignatureRegistry` which will eventually allow for the removal
  of most `&mut self` method on `Compiler`.
@alexcrichton alexcrichton mentioned this issue Feb 1, 2020
Closed
4 tasks
@gnunicorn gnunicorn mentioned this issue Mar 4, 2020
Merged
@arkpar arkpar mentioned this issue Mar 4, 2020
Closed
@alexcrichton alexcrichton mentioned this issue Apr 28, 2020
Merged
@Demi-Marie
Copy link

@alexcrichton why is this impossible?

@alexcrichton
Copy link
Member Author

@demimarie-parity the way things are set up right now you instantiate Instance structures into a Store. The Store keeps a handle on each Instance, so effectively a Store stores each Instance. This is done to keep each Instance alive since we don't have a GC right now to otherwise understand when there are no references to an Instance.

Instances themselves are fundamentally not Sync because operations like global get/set aren't atomic. They're ideally Send, however, but with the Rc that we're using for memory management that forces them to not be Send.

The current state of affairs is that basically Store stores enough instance-local information that it can't be shared across threads because instances can't be shared across threads. There may still be internal refactorings or possible designs to fix this, but overall our threading story isn't great right now unfortunately.

@Demi-Marie
Copy link

@alexcrichton what if we used Arc instead? I imagine that reference count operations are not in hot paths.

@alexcrichton
Copy link
Member Author

Unfortunatey it's not quite as easy as that. In Rust there's no way to statically have a reference counted type that's Send but not Sync, which is morally what we'd want here. The only static solution for that is to entirely remove the reference counting, but that doesn't jive well with the memory management that we have in wasmtime today.

Failing that we'd have to invent our own solution in wasmtime which is much more invasive than just using an Arc instead of an Rc, and I'm not really sure if that's even possible much less workable.

@Demi-Marie
Copy link

What would need to change to allow JIT-compiled code to be used by multiple threads simultaneously?

@alexcrichton
Copy link
Member Author

Ah yes so sharing Store is its own issue but sharing compiled code is definitely something that we want to enable. Making Module both Send and Sync should be easier than making Store either Send or Sync, and that's largely just a matter of internal refactorings.

@yurydelendik
Copy link
Contributor

FWIW wasm-c-api planed to shared a module between two stores via wasm_module_share/wasm_module_obtain , see https://github.com/WebAssembly/wasm-c-api/blob/master/example/threads.c#L34

@Waridley Waridley mentioned this issue Sep 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Milestone
No milestone
3 participants
@alexcrichton @yurydelendik @Demi-Marie