yurydelendik opened PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompileModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompileModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompileModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompileModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik requested alexcrichton for a review on PR #1761.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompileModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompileModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompileModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompileModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompileModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompileModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompileModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompileModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik edited PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompiledModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
- Preliminary marked
CompiledModule
asSend
+Sync
for "frame_info.rs" -- probably needs to be changedStore
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik edited PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
The
Store
here internally looks to be the only reason that this is non-Send
, right? If that's the case can we move theStore
out, or better yet only take anEngine
to compile amodule
?This would require taking a
Store
argument to create anInstance
but I think that's ok sinceLinker
is the "ergonomic" way to create instances.Additionally this would require, on instantiation, registration of JIT code ranges within a
Store
if a module hasn't previously been registered there, but I think that's ok?
alexcrichton created PR Review Comment:
FWIW this is breaking on Windows, but I think it's fine to just skip this example and compile it as a no-op on Windows
alexcrichton created PR Review Comment:
I'm personaly pretty wary of
Deref
unless it's used for smart pointers, so could this be removed to instead useself.module
where necessary?
alexcrichton created PR Review Comment:
This makes me a little uncomfortable using
Deref
like this.The only reason for this, I believe, is keeping the
CodeMemory
alive, right? Since the memory management ofInstanceHandle
is detached from the handle itself (it happens inStore
for thewasmtime
crate), could the managment ofCodeMemory
move intoStore
as well?For example could each
CompiledModule
store anArc<CodeMemory>
, and then we push a strong reference of that into aStore
whenever a module is instantiated within aStore
?
alexcrichton created PR Review Comment:
.find(...).is_some()
can be replaced with.any(...)
as well
alexcrichton created PR Review Comment:
IIRC
SignatureRegistry
has a lock internally, could that be removed now in favor of just using theRefCell
here for mutability?I think that there's no need for
SignatureRegistry
to be synchronized, even in a shared memory world I think instances are still disconnected in that they can have different signature registries on each thread.
alexcrichton created PR Review Comment:
I think in general this probably isn't necessary if we do this registration at instantiation time instead of compile time for a
Module
. Once aModule
is instantiated into aStore
we never drop theInstanceHandle
anyway, so it's permanently registered within a store, which means there's no need to remove.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
I replaced
Deref<Target = Module>
withInstanceContext
. I'm not convinced thatStore
is good location forCodeMemory
. I understand that the end goal is to be capable to ditch entireModule
/CodeModule
even ifInstance
or its exports are present, but keepCodeMemory
alive. Can we do it in follow up PRs?
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik has marked PR #1761 as ready for review.
yurydelendik requested alexcrichton for a review on PR #1761.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
I redesigned API to remove
Store
fromModule
. @alexcrichton can you take a look at this change before I proceed any further?
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Since the
store
is now passed int toInstance::new
I think this check for same-store-ness can be removed?
alexcrichton created PR Review Comment:
One of the worries I have about this is that while an instance is alive it retains the entire
CompiledModule
. Not everything inCompiledModule
, however, is used throughout the liftime of the instance. For example I think a number of internal maps here are also copied to theInstanceHandle
. I think it'd be best if we could split this into two parts:
- Parts needed for the lifetime of the
InstanceHandle
. Some of this is required by the handle itself (likeModule
), others are simply required to stay alive for the duration of the instance (e.g.CodeMemory
andGdbJitImageRegistration
- Other parts are only needed for introspection and instantiation. For example I think the
trampolines
map andfinished_functions
lists here are only needed temporarily, since their relevant bits are copied into theInstanceHandle
.This is a relatively niche concern though and it's not like it's a radical change from what we have here. Could a FIXME and/or issue be left though for this future refactoring?
alexcrichton created PR Review Comment:
I think this can probably be removed since it's only used for the one check in
wasm_instance_new
which I think we can also remove now too.
alexcrichton created PR Review Comment:
Could the deallocation that happens inside here happen outside of the logkc?
alexcrichton created PR Review Comment:
IIRC
let _ = foo
is a special idiom in Rust where it will run the dtor forfoo
immediately, rather than defer it to the end of the block. I think that means that the lock isn't held for the duration of this block?
alexcrichton created PR Review Comment:
Could this allocation happen before the synchronization of the lock?
alexcrichton created PR Review Comment:
I don't think that the
u32
here is used, right?Also could this have documentation as to its purpose?
alexcrichton created PR Review Comment:
Could an instance of
Compiler
be stored within theEngine
itself, so the same instance is reused across all compilations? I thinkCompiler
should beSend
andSync
now right?
alexcrichton created PR Review Comment:
How come this changed to
CompiledModule
? That's retaining a lot more information here thanModule
previously was
alexcrichton created PR Review Comment:
Also as another note, this technically is still unsafe since we're not synchronized with anything else in the process that may be frobbing these symbols. Is there really no way we can rely on a gdb thing to do the synchronization for us?
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
No. This process shall control access to __jit_debug_descriptor -- added comment
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
The gdb only known about
__jit_debug_descriptor
and sets breakpoint at__jit_debug_register_code
. Locking access to __jit_debug_descriptor is the process chore -- gdb will traverse its data once process in the __jit_debug_register_code.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik created PR Review Comment:
Theoretically, yes, we can remove. It will drive wasmtime_instance_new signature change too, and renders wasm_module_share / wasm_module_obtain useless. I would like to defer this change for future work.
yurydelendik submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
This might be best as a private method on
Config
?
alexcrichton created PR Review Comment:
Could these two fields be wrapped up in one
Arc
allocation?
alexcrichton created PR Review Comment:
Could
Default for Store
delegate to this now that it exists?
alexcrichton created PR Review Comment:
FWIW now that we're doubling down on
Rc
inside ofStore
, I think we should remove these methods. It's too easy, at callsites, to forget these methods are related toRefCell
and can introduce panics.
alexcrichton created PR Review Comment:
Or, alternatively, all accesses to these fields would be scoped inside of methods on this type, where each method doesn't leak borrowed
RefCell
s through returns
alexcrichton created PR Review Comment:
Could this have a comment for what's going on here? Without that it's a bit cryptic what this check is doing
alexcrichton created PR Review Comment:
I think changing signatures is fine, but if you'd prefer to defer that's ok too. Can you be sure an issue is filed about this?
alexcrichton created PR Review Comment:
(removed in favor of direct field access at callsites, that is)
alexcrichton created PR Review Comment:
Er sorry I mean thte literal call to malloc, not the reading/writing of the global.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
Returned back to store only Module, and added ModuleCode (which is only CodeMemory + GdbJitRegistration)
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik submitted PR Review.
yurydelendik created PR Review Comment:
Not sure how to do it:
Engine::new
andStore::new
have some additional logic that needs to be run.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik requested alexcrichton for a review on PR #1761.
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Long-term it feels weird to have two
dyn Any
in thisInstance
, instead ideally we'd just have oneBox<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.
alexcrichton created PR Review Comment:
Instead of embedding the
.unwrap()
here, could this returnOption<&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
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik updated PR #1761 from mv-sig-registry
to master
:
To make wasmtime more threads friendly, this PR:
- Moves
CodeMemory
,VMInterrupts
andSignatureRegistry
fromCompiler
CompiledModule
holdsCodeMemory
andGdbJitImageRegistration
Store
keeps track of its JIT code- Makes "jit_int.rs" stuff
Send
+Sync
- Adds the threads example.
yurydelendik merged PR #1761.
Last updated: Nov 22 2024 at 16:03 UTC