-
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
Changes from all commits
301540c
b7ab85d
3809420
5890423
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I understand when this can be triggered. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
/// Exports is a special kind of map that allows easily unwrapping | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. yeah this is a easy one to bring in |
||
T: Any + Send + 'static + Sized, | ||
{ | ||
|
@@ -112,6 +112,11 @@ impl<T: Send + 'static> FunctionEnvMut<'_, T> { | |
self.func_env.as_mut(&mut self.store_mut) | ||
} | ||
|
||
/// Borrows a new immmutable reference | ||
pub fn as_ref(&self) -> FunctionEnv<T> { | ||
self.func_env.clone() | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. yeah if we move the other ones we might as well |
||
/// Borrows a new mutable reference | ||
pub fn as_mut<'a>(&'a mut self) -> FunctionEnvMut<'a, T> { | ||
FunctionEnvMut { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,6 @@ use std::fmt; | |
pub struct Instance { | ||
_handle: StoreHandle<WebAssembly::Instance>, | ||
module: Module, | ||
#[allow(dead_code)] | ||
imports: Imports, | ||
/// The exports for an instance. | ||
pub exports: Exports, | ||
} | ||
|
@@ -65,12 +63,11 @@ impl Instance { | |
module: &Module, | ||
imports: &Imports, | ||
) -> Result<Self, InstantiationError> { | ||
let import_copy = imports.clone(); | ||
let (instance, _imports): (StoreHandle<WebAssembly::Instance>, Vec<Extern>) = module | ||
let (instance, externs): (StoreHandle<WebAssembly::Instance>, Vec<Extern>) = module | ||
.instantiate(&mut store, imports) | ||
.map_err(|e| InstantiationError::Start(e))?; | ||
|
||
let self_instance = Self::from_module_and_instance(store, module, instance, import_copy)?; | ||
let self_instance = Self::from_module_and_instance(store, module, externs, instance)?; | ||
//self_instance.init_envs(&imports.iter().map(Extern::to_export).collect::<Vec<_>>())?; | ||
Ok(self_instance) | ||
} | ||
|
@@ -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 commentThe 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 commentThe 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 commentThe 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? |
||
imports: Imports, | ||
) -> Result<Self, InstantiationError> { | ||
let instance_exports = instance.get(store.as_store_ref().objects()).exports(); | ||
let exports = module | ||
let mut exports = module | ||
.exports() | ||
.map(|export_type| { | ||
let name = export_type.name(); | ||
|
@@ -110,10 +107,17 @@ impl Instance { | |
}) | ||
.collect::<Result<Exports, InstantiationError>>()?; | ||
|
||
// 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()); | ||
} | ||
} | ||
Comment on lines
+110
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
Ok(Self { | ||
_handle: instance, | ||
module: module.clone(), | ||
imports, | ||
exports, | ||
}) | ||
} | ||
|
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