-
Notifications
You must be signed in to change notification settings - Fork 824
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
Multithreading, full networking and RPC for WebAssembly #3116
Multithreading, full networking and RPC for WebAssembly #3116
Conversation
c34d4a1
to
680043e
Compare
@@ -53,6 +53,9 @@ pub enum ExportError { | |||
/// This error arises when an export is missing | |||
#[error("Missing export {0}")] | |||
Missing(String), | |||
/// This error arises when an export is missing | |||
#[error("Serialization failed {0}")] | |||
SerializationFailed(String), |
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.
Not sure if I understand when this can be triggered.
When an export can be missing?
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.
ah this is leftovers - this is not used anymore - we should remove it
// If the memory is imported then also export it for backwards compatibility reasons | ||
// (many will assume the memory is always exported) - later we can remove this | ||
if exports.get_memory("memory").is_err() { | ||
if let Some(memory) = externs.iter().filter(|a| a.ty(store).memory().is_some()).next() { | ||
exports.insert("memory", memory.clone()); | ||
} | ||
} |
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 really believe we shouldn't do this on the API layer. This should live somewhere else (not on the JS API layer, as it's not JS or VM specific)
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.
|
||
```rust | ||
let mut wasi_env = WasiState::new("hello").finalize()?; | ||
let import_object = wasi_env.import_object(&mut store, &module)?; | ||
let instance = Instance::new(&mut store, &module, &import_object).expect("Could not instantiate module."); | ||
let memory = instance.exports.get_memory("memory")?; | ||
wasi_env.data_mut(&mut store).set_memory(memory.clone()); | ||
wasi_env.initialize(&mut store, &instance).unwrap(); |
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.
Maybe we will want to do this change in the 3.0
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 i think it would be good to bring this forward - i think this way is cleaner
@@ -36,7 +36,7 @@ impl<T> FunctionEnv<T> { | |||
} | |||
|
|||
/// Get the data as reference | |||
pub fn as_ref<'a>(&self, store: &'a impl AsStoreMut) -> &'a T | |||
pub fn as_ref<'a>(&self, store: &'a impl AsStoreRef) -> &'a T | |||
where |
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.
That could also go to 3.0
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.
yeah this is a easy one to bring in
pub fn as_ref(&self) -> FunctionEnv<T> { | ||
self.func_env.clone() | ||
} | ||
|
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 that could go to 3.0 too, I suppose
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.
yeah if we move the other ones we might as well
@@ -87,11 +84,11 @@ impl Instance { | |||
pub fn from_module_and_instance( | |||
mut store: &mut impl AsStoreMut, | |||
module: &Module, | |||
externs: Vec<Extern>, | |||
instance: StoreHandle<WebAssembly::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.
That's another API change that should go in 3.0?
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.
not sure on this one as it will drag a bunch of other code with it - might be too much
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.
Does that mean the signature of the function can be rolled back to without the externs, and we forget about this change?
@@ -152,7 +186,8 @@ impl Module { | |||
/// Opposed to [`Module::new`], this function is not compatible with | |||
/// the WebAssembly text format (if the "wat" feature is enabled for | |||
/// this crate). | |||
pub fn from_binary(_store: &impl AsStoreRef, binary: &[u8]) -> Result<Self, CompileError> { | |||
pub fn from_binary(_store: &impl AsStoreRef, binary: impl IntoBytes) -> Result<Self, CompileError> { | |||
let binary = binary.into_bytes(); | |||
// |
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 all the IntoBytes
changes can also be consider API-breaking changes? (and so should go in 3.0?)
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.
Why we want to own the bytes @john-sharratt ?
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.
In order to pass it between threads - in particular it needs to be shipped to the other WebWorkers in the browser as they can not share the modules between each other, only the compiled bytes can be shared.
@@ -29,7 +29,7 @@ impl<T> FunctionEnv<T> { | |||
} | |||
|
|||
/// Get the data as reference | |||
pub fn as_ref<'a>(&self, store: &'a impl AsStoreMut) -> &'a T | |||
pub fn as_ref<'a>(&self, store: &'a impl AsStoreRef) -> &'a 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.
Should also go in 3.0
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.
agreed
if let Some(memory) = externs.iter().filter(|a| a.ty(store).memory().is_some()).next() { | ||
exports.insert("memory", memory.clone()); | ||
} | ||
} | ||
|
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.
does this change is also a break-change on the API level?
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.
this probably needs some more thought on it anyway as it assumes the first exported memory but in the future there could be multiple - it does not change the API but does change the behavior.
@@ -76,7 +76,7 @@ impl<T, M: MemorySize> WasmPtr<T, M> { | |||
|
|||
/// Get the offset into Wasm linear memory for this `WasmPtr`. | |||
#[inline] | |||
pub fn offset(self) -> M::Offset { | |||
pub fn offset(&self) -> M::Offset { | |||
self.offset |
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.
API breaking 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.
API change but i dont think its breaking
_store: &impl AsStoreRef, | ||
binary: &[u8], | ||
store: &impl AsStoreRef, | ||
binary: impl IntoBytes, |
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.
Why we want to own the bytes @john-sharratt ?
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.
so the compiled code can be shared to the main thread, compiled there and fed to the other web workers. its a complex setup to get it working because of the JS limitations
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.
if there is an easier way it would be good to drop the Bytes but it is tricky, thats for sure
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.
In the case of JS, the bytes are already stored in Vec<u8>
. So it shall be possible to share the compiled code?
@@ -210,8 +257,9 @@ impl Module { | |||
/// This validation is normally pretty fast and checks the enabled | |||
/// WebAssembly features in the Store Engine to assure deterministic | |||
/// validation of the Module. | |||
pub fn validate(_store: &impl AsStoreRef, binary: &[u8]) -> Result<(), CompileError> { | |||
let js_bytes = unsafe { Uint8Array::view(binary) }; | |||
pub fn validate(_store: &impl AsStoreRef, binary: impl IntoBytes) -> Result<(), CompileError> { |
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.
Why we want to own the bytes @john-sharratt ?
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.
see above
Behold! Multithreading, networking and RPC!