Stream: git-wasmtime

Topic: wasmtime / PR #1761 Disconnects `Store` state fields from...


view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 19:54):

yurydelendik opened PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 20:00):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 20:00):

yurydelendik requested alexcrichton for a review on PR #1761.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 21:21):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 21:30):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 21:34):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 21:56):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 21:59):

yurydelendik edited PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 22:23):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 22:23):

yurydelendik edited PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 22:50):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 22:58):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

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 the Store out, or better yet only take an Engine to compile a module?

This would require taking a Store argument to create an Instance but I think that's ok since Linker 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?

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

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

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

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 use self.module where necessary?

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

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 of InstanceHandle is detached from the handle itself (it happens in Store for the wasmtime crate), could the managment of CodeMemory move into Store as well?

For example could each CompiledModule store an Arc<CodeMemory>, and then we push a strong reference of that into a Store whenever a module is instantiated within a Store?

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

alexcrichton created PR Review Comment:

.find(...).is_some() can be replaced with .any(...) as well

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

alexcrichton created PR Review Comment:

IIRC SignatureRegistry has a lock internally, could that be removed now in favor of just using the RefCell 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.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 15:08):

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 a Module is instantiated into a Store we never drop the InstanceHandle anyway, so it's permanently registered within a store, which means there's no need to remove.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 16:03):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 16:22):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 17:16):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 17:17):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 17:22):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 17:22):

yurydelendik created PR Review Comment:

I replaced Deref<Target = Module> with InstanceContext. I'm not convinced that Store is good location for CodeMemory. I understand that the end goal is to be capable to ditch entire Module/CodeModule even if Instance or its exports are present, but keep CodeMemory alive. Can we do it in follow up PRs?

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 19:37):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 19:37):

yurydelendik has marked PR #1761 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 19:37):

yurydelendik requested alexcrichton for a review on PR #1761.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 19:39):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 19:39):

yurydelendik created PR Review Comment:

I redesigned API to remove Store from Module. @alexcrichton can you take a look at this change before I proceed any further?

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 19:55):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2020 at 21:12):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton created PR Review Comment:

Since the store is now passed int to Instance::new I think this check for same-store-ness can be removed?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

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 in CompiledModule, however, is used throughout the liftime of the instance. For example I think a number of internal maps here are also copied to the InstanceHandle. I think it'd be best if we could split this into two parts:

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?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

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.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton created PR Review Comment:

Could the deallocation that happens inside here happen outside of the logkc?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton created PR Review Comment:

IIRC let _ = foo is a special idiom in Rust where it will run the dtor for foo 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?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton created PR Review Comment:

Could this allocation happen before the synchronization of the lock?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

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?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton created PR Review Comment:

Could an instance of Compiler be stored within the Engine itself, so the same instance is reused across all compilations? I think Compiler should be Send and Sync now right?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

alexcrichton created PR Review Comment:

How come this changed to CompiledModule? That's retaining a lot more information here than Module previously was

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 16:06):

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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 15:27):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 15:27):

yurydelendik created PR Review Comment:

No. This process shall control access to __jit_debug_descriptor -- added comment

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 15:27):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 15:32):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 15:32):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 15:36):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 16:22):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 18:40):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 19:03):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 19:03):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton created PR Review Comment:

This might be best as a private method on Config?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton created PR Review Comment:

Could these two fields be wrapped up in one Arc allocation?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton created PR Review Comment:

FWIW now that we're doubling down on Rc inside of Store, I think we should remove these methods. It's too easy, at callsites, to forget these methods are related to RefCell and can introduce panics.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

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 RefCells through returns

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton created PR Review Comment:

(removed in favor of direct field access at callsites, that is)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:24):

alexcrichton created PR Review Comment:

Er sorry I mean thte literal call to malloc, not the reading/writing of the global.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:28):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:33):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:33):

yurydelendik created PR Review Comment:

Returned back to store only Module, and added ModuleCode (which is only CodeMemory + GdbJitRegistration)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 20:57):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 21:53):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 22:05):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 22:05):

yurydelendik created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 22:06):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 22:32):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 22:38):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 23:26):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 23:27):

yurydelendik requested alexcrichton for a review on PR #1761.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 17:00):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 17:00):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 17:00):

alexcrichton created PR Review Comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 17:00):

alexcrichton created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 17:44):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 18:12):

yurydelendik updated PR #1761 from mv-sig-registry to master:

To make wasmtime more threads friendly, this PR:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 02 2020 at 18:44):

yurydelendik merged PR #1761.


Last updated: Nov 22 2024 at 16:03 UTC