Implement Send for Instance #807

MarkMcCaskey · 2019-09-17T17:38:45Z

Review

Create a short description of the the change in the CHANGELOG.md file

Resolves #748

WIP

List of changes

Commit 1

Globals use Arc instead of RC
Export Context and FuncPointer manually implement Send
ImportObject uses Arc<Mutex<HashMap<...>>> Instead of Rc<RefCell<HashMap<...>>>; removed get_namespace method in favor of continuation style to deal with locking the Mutex
Func manually implements Send (TODO: this change needs to be checked in depth)

Commit 2

unsafe impl Send for Export {} (temporary to allow Memory to be not Send)
RefCell -> Mutex in Global and Table
Rc -> Arc in Table
Namespace's IsExports must be Send (Done to avoid touching much more of the code (i.e. trait IsExport: Send requires a lot -- maybe this is what we should do though)
Make Host and Wasm Funcs Send (manual implementation)
Manual implementation for LocalTable and AnyFunc

Commit 3

rm placeholder unsafe impl Send for Export {}
Manual implementation for LocalBacking and ImportBacking (both seemed to be not Send due to direct ownership of mutable pointers in their containers)
ImportObject's state creator Fn trait object is now + Send + Sync + 'static (required because it's in an Arc)
Manually implement Send for InstanceInner because it holds a raw pointer, LocalBacking and ImportBacking are marked Send separately
Memory: All Rc -> Arc (including unshared memory); All RefCell -> Mutex (including unshared memory)
Manual implementation of Send and Sync on UnsharedMemoryInternal
Manual implementation of Send and Sync on SharedMemoryInternal
Change runtime-core::unix::memory::Memory.fd from Option<Rc<Rawfd>> to Option<Arc<Rawfd>> (not strictly required for this change because Memory has manual implementations of Send and Sync, but Arc seems more correct here and there's no comment justifying the use of Rc)
Manual implementation of Send for ImportedFunc

lib/runtime-core/src/import.rs

lib/runtime-core/src/export.rs

nlewycky · 2019-09-18T00:44:04Z

lib/runtime-core/src/export.rs

@@ -26,6 +28,8 @@ pub enum Export {
 #[derive(Debug, Clone)]
 pub struct FuncPointer(*const vm::Func);

+unsafe impl Send for FuncPointer {}


For this in particular, I think it's relevant that we don't have any thread-local variables.

Hmm, can you explain what you mean? I agree that thread-local variables can't be Send, is that something we've done or talked about in the past with FuncPointer?

bjfish · 2019-09-21T19:25:27Z

It would be nice to add a test like this for module/instance:

wasmer/lib/runtime-core/src/instance.rs

Lines 736 to 745 in 801339a

    
           #[cfg(test)] 
        
           mod test { 
        
               use super::*; 
        
               fn is_send<T: Send>() {} 
        
               #[test] 
        
               fn test_instance_is_send() { 
        
                   is_send::<Instance>(); 
        
               }

…ce-send

MarkMcCaskey · 2019-09-23T18:20:26Z

bors r+

807: Implement Send for Instance r=MarkMcCaskey a=MarkMcCaskey # Review - [x] Create a short description of the the change in the CHANGELOG.md file Resolves #748 WIP ## List of changes ### Commit 1 - `Global`s use Arc instead of RC - Export `Context` and `FuncPointer` manually implement Send - `ImportObject` uses `Arc<Mutex<HashMap<...>>>` Instead of `Rc<RefCell<HashMap<...>>>`; removed `get_namespace` method in favor of continuation style to deal with locking the Mutex - `Func` manually implements `Send` (TODO: this change needs to be checked in depth) ### Commit 2 - `unsafe impl Send for Export {}` (temporary to allow Memory to be not Send) - RefCell -> Mutex in Global and Table - Rc -> Arc in Table - Namespace's `IsExport`s must be `Send` (Done to avoid touching much more of the code (i.e. `trait IsExport: Send` requires a lot -- maybe this is what we should do though) - Make `Host` and `Wasm` `Func`s Send (manual implementation) - Manual implementation for `LocalTable` and `AnyFunc` ### Commit 3 - rm placeholder `unsafe impl Send for Export {}` - Manual implementation for `LocalBacking` and `ImportBacking` (both seemed to be not Send due to direct ownership of mutable pointers in their containers) - ImportObject's state creator Fn trait object is now ` + Send + Sync + 'static` (required because it's in an Arc) - Manually implement Send for `InstanceInner` because it holds a raw pointer, `LocalBacking` and `ImportBacking` are marked Send separately - Memory: All Rc -> Arc (including unshared memory); All RefCell -> Mutex (including unshared memory) - Manual implementation of Send and Sync on `UnsharedMemoryInternal` - Manual implementation of Send and Sync on `SharedMemoryInternal` - Change `runtime-core::unix::memory::Memory.fd` from `Option<Rc<Rawfd>>` to `Option<Arc<Rawfd>>` (not strictly required for this change because Memory has manual implementations of Send and Sync, but Arc seems more correct here and there's no comment justifying the use of Rc) - Manual implementation of Send for `ImportedFunc` Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>

bors · 2019-09-23T18:47:01Z

Build failed

wasmerio.wasmer

…er into feature/make-instance-send

MarkMcCaskey · 2019-09-23T20:44:06Z

bors try

MarkMcCaskey · 2019-09-23T21:19:45Z

bors r+

807: Implement Send for Instance r=MarkMcCaskey a=MarkMcCaskey # Review - [x] Create a short description of the the change in the CHANGELOG.md file Resolves #748 WIP ## List of changes ### Commit 1 - `Global`s use Arc instead of RC - Export `Context` and `FuncPointer` manually implement Send - `ImportObject` uses `Arc<Mutex<HashMap<...>>>` Instead of `Rc<RefCell<HashMap<...>>>`; removed `get_namespace` method in favor of continuation style to deal with locking the Mutex - `Func` manually implements `Send` (TODO: this change needs to be checked in depth) ### Commit 2 - `unsafe impl Send for Export {}` (temporary to allow Memory to be not Send) - RefCell -> Mutex in Global and Table - Rc -> Arc in Table - Namespace's `IsExport`s must be `Send` (Done to avoid touching much more of the code (i.e. `trait IsExport: Send` requires a lot -- maybe this is what we should do though) - Make `Host` and `Wasm` `Func`s Send (manual implementation) - Manual implementation for `LocalTable` and `AnyFunc` ### Commit 3 - rm placeholder `unsafe impl Send for Export {}` - Manual implementation for `LocalBacking` and `ImportBacking` (both seemed to be not Send due to direct ownership of mutable pointers in their containers) - ImportObject's state creator Fn trait object is now ` + Send + Sync + 'static` (required because it's in an Arc) - Manually implement Send for `InstanceInner` because it holds a raw pointer, `LocalBacking` and `ImportBacking` are marked Send separately - Memory: All Rc -> Arc (including unshared memory); All RefCell -> Mutex (including unshared memory) - Manual implementation of Send and Sync on `UnsharedMemoryInternal` - Manual implementation of Send and Sync on `SharedMemoryInternal` - Change `runtime-core::unix::memory::Memory.fd` from `Option<Rc<Rawfd>>` to `Option<Arc<Rawfd>>` (not strictly required for this change because Memory has manual implementations of Send and Sync, but Arc seems more correct here and there's no comment justifying the use of Rc) - Manual implementation of Send for `ImportedFunc` Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>

bors · 2019-09-23T21:44:08Z

try

Build succeeded

wasmerio.wasmer

bors · 2019-09-23T21:46:28Z

Build succeeded

wasmerio.wasmer

865: adding tests for `state_creator` of `import_object` r=MarkMcCaskey a=YaronWittenstein Part of the PR #807 changes was adding support for shared import objects between threads. https://github.com/wasmerio/wasmer/pull/807/files#diff-d20cb4c5a883566b85be4cc046f45aa9R49 I've added tests/examples on how to create an `import object` with a state_creator (function or closure) Co-authored-by: Yaron Wittenstein <[email protected]>

709: new feature flag: `deterministic` r=MarkMcCaskey a=YaronWittenstein The motivation for the PR is for introducing a new feature flag called `deterministic`. When `deterministic` will be enabled (turned-off by default) it'll guarantee deterministic execution of wasm programs across different hardware/circumstances. This is critical for Blockchain projects that require execution to be deterministic in order to reach a consensus of the state transition of each smart-contract transaction. 865: adding tests for `state_creator` of `import_object` r=MarkMcCaskey a=YaronWittenstein Part of the PR #807 changes was adding support for shared import objects between threads. https://github.com/wasmerio/wasmer/pull/807/files#diff-d20cb4c5a883566b85be4cc046f45aa9R49 I've added tests/examples on how to create an `import object` with a state_creator (function or closure) 1042: Make regression test work in release builds too. r=nlewycky a=nlewycky Fix this regression test to detect the bug it was looking for in release builds too. This bug triggered an assertion failure in debug, and by examining the pre-opt IR, we can check for the bug in release mode too. Co-authored-by: Yaron Wittenstein <[email protected]> Co-authored-by: Yaron Wittenstein <[email protected]> Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Nick Lewycky <[email protected]>

865: adding tests for `state_creator` of `import_object` r=MarkMcCaskey a=YaronWittenstein Part of the PR #807 changes was adding support for shared import objects between threads. https://github.com/wasmerio/wasmer/pull/807/files#diff-d20cb4c5a883566b85be4cc046f45aa9R49 I've added tests/examples on how to create an `import object` with a state_creator (function or closure) 1042: Make regression test work in release builds too. r=nlewycky a=nlewycky Fix this regression test to detect the bug it was looking for in release builds too. This bug triggered an assertion failure in debug, and by examining the pre-opt IR, we can check for the bug in release mode too. Co-authored-by: Yaron Wittenstein <[email protected]> Co-authored-by: Nick Lewycky <[email protected]>

865: adding tests for `state_creator` of `import_object` r=MarkMcCaskey a=YaronWittenstein Part of the PR #807 changes was adding support for shared import objects between threads. https://github.com/wasmerio/wasmer/pull/807/files#diff-d20cb4c5a883566b85be4cc046f45aa9R49 I've added tests/examples on how to create an `import object` with a state_creator (function or closure) Co-authored-by: Yaron Wittenstein <[email protected]>

865: adding tests for `state_creator` of `import_object` r=syrusakbary a=YaronWittenstein Part of the PR #807 changes was adding support for shared import objects between threads. https://github.com/wasmerio/wasmer/pull/807/files#diff-d20cb4c5a883566b85be4cc046f45aa9R49 I've added tests/examples on how to create an `import object` with a state_creator (function or closure) Co-authored-by: Yaron Wittenstein <[email protected]> Co-authored-by: Syrus Akbary <[email protected]>

Implement Send for Instance

17a0e78

MarkMcCaskey added the 📦 lib-deprecated About the deprecated crates label Sep 17, 2019

Mark McCaskey added 2 commits September 17, 2019 11:45

Implement Send for everything except Memory

9e93438

Implement it for memory and make Instance Send

83c3909

MarkMcCaskey requested review from bjfish, losfair, Hywan and nlewycky September 17, 2019 23:42

nlewycky reviewed Sep 18, 2019

View reviewed changes

Add comments explaining the unsafe impls and simplify the code, too

9c205e0

Hywan added the 🎉 enhancement New feature! label Sep 18, 2019

MarkMcCaskey mentioned this pull request Sep 20, 2019

Ctx needs ability to invoke functions #638

Closed

MarkMcCaskey marked this pull request as ready for review September 20, 2019 22:51

bjfish mentioned this pull request Sep 21, 2019

Add tests to check that module and instance are send #573

Closed

Mark McCaskey and others added 3 commits September 23, 2019 11:04

Add test for Instance, fix tests for ImportObject

05ad1aa

Merge remote-tracking branch 'origin/master' into feature/make-instan…

1e637ba

…ce-send

Merge branch 'master' into feature/make-instance-send

705287c

syrusakbary approved these changes Sep 23, 2019

View reviewed changes

Mark McCaskey added 2 commits September 23, 2019 13:43

Update spectests to work with new Instance; use Arc<Mutex<>>

c4818f1

Merge branch 'feature/make-instance-send' of github.com:wasmerio/wasm…

9cab6d7

…er into feature/make-instance-send

bors bot added a commit that referenced this pull request Sep 23, 2019

Try #807:

95e7e26

nlewycky approved these changes Sep 23, 2019

View reviewed changes

bors bot merged commit 9cab6d7 into master Sep 23, 2019

bors bot deleted the feature/make-instance-send branch September 23, 2019 21:46

syrusakbary mentioned this pull request Oct 2, 2019

Try to remove sync + send traits on implementations andrewdavidmackenzie/flow#295

Closed

YaronWittenstein mentioned this pull request Oct 7, 2019

adding tests for state_creator of import_object #865

Closed

Hywan mentioned this pull request Nov 18, 2019

Using wasmer from goroutines wasmerio/wasmer-go#84

Closed

dhedey mentioned this pull request Nov 23, 2022

Feature/engine no std radixdlt/radixdlt-scrypto#595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Send for Instance #807

Implement Send for Instance #807

MarkMcCaskey commented Sep 17, 2019 •

edited

Loading

nlewycky Sep 18, 2019

MarkMcCaskey Sep 18, 2019

bjfish commented Sep 21, 2019

MarkMcCaskey commented Sep 23, 2019

bors bot commented Sep 23, 2019

MarkMcCaskey commented Sep 23, 2019

MarkMcCaskey commented Sep 23, 2019

bors bot commented Sep 23, 2019

bors bot commented Sep 23, 2019

Implement Send for Instance #807

Implement Send for Instance #807

Conversation

MarkMcCaskey commented Sep 17, 2019 • edited Loading

Review

List of changes

Commit 1

Commit 2

Commit 3

nlewycky Sep 18, 2019

Choose a reason for hiding this comment

MarkMcCaskey Sep 18, 2019

Choose a reason for hiding this comment

bjfish commented Sep 21, 2019

MarkMcCaskey commented Sep 23, 2019

bors bot commented Sep 23, 2019

Build failed

MarkMcCaskey commented Sep 23, 2019

MarkMcCaskey commented Sep 23, 2019

bors bot commented Sep 23, 2019

try

Build succeeded

bors bot commented Sep 23, 2019

Build succeeded

MarkMcCaskey commented Sep 17, 2019 •

edited

Loading