Disconnects `Store` state fields from `Compiler` #1761

yurydelendik · 2020-05-26T19:54:19Z

To make wasmtime more threads friendly, this PR:

Moves CodeMemory, VMInterrupts and SignatureRegistry from Compiler
CompiledModule holds CodeMemory and GdbJitImageRegistration
Store keeps track of its JIT code
Makes "jit_int.rs" stuff Send+Sync
Adds the threads example.

github-actions · 2020-05-26T20:05:29Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2020-05-26T21:47:31Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

alexcrichton

Nice!

This is looking really promising I think. I've left some various comments about organization below.

Also, would you be up for adding some more tests about using threads? I think it'd be good to try to start adding various tests here and there that try to stress the multithreaded aspects.

crates/jit/src/instantiate.rs

examples/threads.c

crates/wasmtime/src/module.rs

crates/runtime/src/instance.rs

crates/wasmtime/src/runtime.rs

github-actions · 2020-05-27T19:55:45Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing", "wasmtime:docs"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

alexcrichton

Looking great, thanks! Some other remaining points I can think of:

Can the documentation be updated that Module is threadsafe and safe to share across threads?
Can a test be specifically added that Module is Send and Sync?
I think we may want some form of checks when we instantiate a module into a store. I'm a bit worried about, for example you compile it with one engine that conflicts with configuration within another engine. For example you could compile it in one engine with interrupts disabled, and then instantiate it in another with interrupts enabled, and then interrupts wouldn't work.

crates/c-api/src/instance.rs

crates/c-api/src/module.rs

crates/jit/src/instantiate.rs

crates/runtime/src/jit_int.rs

crates/wasmtime/src/frame_info.rs

crates/wasmtime/src/runtime.rs

alexcrichton · 2020-06-01T20:18:29Z

crates/wasmtime/src/runtime.rs

+
+impl Default for Engine {
+    fn default() -> Engine {
+        Engine::new(&Config::default())


Could Default for Store delegate to this now that it exists?

Not sure how to do it: Engine::new and Store::new have some additional logic that needs to be run.

crates/wasmtime/src/runtime.rs

crates/c-api/src/module.rs

crates/runtime/src/jit_int.rs

e.g. https://github.com/bytecodealliance/wasmtime/runs/728875254

alexcrichton

FWIW this is a relatively major API change for all users since Module::new and Instance::new are changing. I think the Instance::new change is fine to sweep under the rug because it's largely supplanted by Linker, but Module::new is somewhat serious and you can see the effect it has on all the examples here.

The only way I could see of how to improve things is to implement AsRef<Engine> for Store and then take impl AsRef<Engine> in the constructor for Module. That way folks still wouldn't need to deal with Engine in general.

I don't think it's necessary to do that in this PR, however. I think it's fine to leave this for a later date if truly necessary.

alexcrichton · 2020-06-02T16:56:06Z

crates/jit/src/instantiate.rs

-    pub fn module_ref(&self) -> &Module {
-        &self.module
+    pub fn module_mut(&mut self) -> &mut Module {
+        Arc::get_mut(&mut self.module).unwrap()


Instead of embedding the .unwrap() here, could this return Option<&mut Module>? It's only used in one location right now which is fine, but I'd prefer to avoid proliferating users of this API

alexcrichton · 2020-06-02T16:57:35Z

crates/runtime/src/instance.rs

@@ -40,6 +38,9 @@ pub(crate) struct Instance {
    /// The `Module` this `Instance` was instantiated from.
    module: Arc<Module>,

+    /// The module's JIT code (if exists).
+    code: Arc<dyn Any>,


Long-term it feels weird to have two dyn Any in this Instance, instead ideally we'd just have one Box<dyn Any> and shove everything in there. I don't want to iterate too much on this, it's pretty unimportant at this time. Just something to keep in mind for the future, this is more than ok for now.

sunfishcode · 2020-06-02T17:54:32Z

Please add a note to the RELEASES.md describing this change 😄

yurydelendik added 7 commits May 22, 2020 17:29

mv SignatureRegistry

aef8458

mv code_memory

050aebe

Move Arc to CompiledModule level; unsafe Sync+Send

d7defe2

Threads example

5a3a142

rm interrupts from Compiler

9b08221

rm Sync+Send from Module

45717c3

Merge remote-tracking branch 'upstream/master' into mv-sig-registry

dc497a5

github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label May 26, 2020

yurydelendik requested a review from alexcrichton May 26, 2020 20:00

c api

d47371f

yurydelendik force-pushed the mv-sig-registry branch from 1e94ec4 to c9da8fb Compare May 26, 2020 21:34

github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label May 26, 2020

rn module_ref()

d4e4ade

yurydelendik force-pushed the mv-sig-registry branch from c9da8fb to d4e4ade Compare May 26, 2020 21:56

small fixes

a2b432b

abrown mentioned this pull request May 26, 2020

Move SingleFunctionCompiler into its own crate, cranelift-simplecompiler #1756

Closed

yurydelendik added 2 commits May 26, 2020 17:56

rm Send+Sync from CompiledModule

ede93de

rm RawCompiledModule

2e73c4a

yurydelendik force-pushed the mv-sig-registry branch from b54ba6b to 2e73c4a Compare May 26, 2020 22:58

yurydelendik mentioned this pull request May 27, 2020

assertion failed: prev.start > max in wasmtime::frame_info::register #1768

Closed

alexcrichton reviewed May 27, 2020

View reviewed changes

yurydelendik added 5 commits May 27, 2020 11:03

rm jit_code_registration; win32 test; any

edcf487

rm SignatureRegistry lock

3f7c2a5

replace Deref with InstanceContext

1ccdee7

fix test

810ec8a

Redesign API to remove Store from Module::new

3ab0e3c

yurydelendik marked this pull request as ready for review May 27, 2020 19:37

yurydelendik requested a review from alexcrichton May 27, 2020 19:37

fix fuzz

02f79f8

alexcrichton reviewed May 29, 2020

View reviewed changes

yurydelendik added 7 commits June 1, 2020 10:24

jit_int comments; fix 'let _ =' lifetime

2009eb7

Merge remote-tracking branch 'upstream/master' into mv-sig-registry

02bd6f4

Fix linker tests

2f23c7f

Assert Module and Compiler's Sync+Send

bde14b9

One Compiler instance per Engine

3f04e73

Add thread safety doc

6fd50db

check same engine constraint

acc6603

alexcrichton reviewed Jun 1, 2020

View reviewed changes

yurydelendik added 9 commits June 1, 2020 15:28

Reduces amount of held Module data

b3d3eda

Change signatures of wasmtime_module_new/wasmtime_instance_new

6e1a590

mv alloc/drop outside of lock

449cc52

mv build_compiler()

4048b5a

register_jit_code comment

ce9bedf

Remove signatures() accessors where possible

9cf7a61

fix tests

88093a6

revert some changes

fc9d289

Fix intermitent failures in threads tests

4cefa4a

e.g. https://github.com/bytecodealliance/wasmtime/runs/728875254

yurydelendik requested a review from alexcrichton June 1, 2020 23:27

alexcrichton approved these changes Jun 2, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into mv-sig-registry

5fc0a16

yurydelendik added 3 commits June 2, 2020 13:03

mv unwrap(); reduce code churn

65f2c00

Merge remote-tracking branch 'upstream/master' into mv-sig-registry

757ce85

RELEASES.md

430a417

leoyvens mentioned this pull request Jun 2, 2020

Switch the runtime from wasmi to wasmtime + cranelift graphprotocol/graph-node#1700

Merged

yurydelendik merged commit 15c68f2 into bytecodealliance:master Jun 2, 2020

yurydelendik deleted the mv-sig-registry branch June 2, 2020 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disconnects `Store` state fields from `Compiler` #1761

Disconnects `Store` state fields from `Compiler` #1761

yurydelendik commented May 26, 2020 •

edited

Loading

github-actions bot commented May 26, 2020

github-actions bot commented May 26, 2020

alexcrichton left a comment

github-actions bot commented May 27, 2020

alexcrichton left a comment

alexcrichton Jun 1, 2020

yurydelendik Jun 1, 2020

alexcrichton left a comment

alexcrichton Jun 2, 2020

alexcrichton Jun 2, 2020

sunfishcode commented Jun 2, 2020

Disconnects Store state fields from Compiler #1761

Disconnects Store state fields from Compiler #1761

Conversation

yurydelendik commented May 26, 2020 • edited Loading

github-actions bot commented May 26, 2020

Subscribe to Label Action

github-actions bot commented May 26, 2020

Subscribe to Label Action

alexcrichton left a comment

Choose a reason for hiding this comment

github-actions bot commented May 27, 2020

Subscribe to Label Action

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jun 1, 2020

Choose a reason for hiding this comment

yurydelendik Jun 1, 2020

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jun 2, 2020

Choose a reason for hiding this comment

alexcrichton Jun 2, 2020

Choose a reason for hiding this comment

sunfishcode commented Jun 2, 2020

Disconnects `Store` state fields from `Compiler` #1761

Disconnects `Store` state fields from `Compiler` #1761

yurydelendik commented May 26, 2020 •

edited

Loading