Stream: git-wasmtime

Topic: wasmtime / PR #2842 Additional performance improvements f...

Wasmtime GitHub notifications bot (Apr 14 2021 at 23:44):

peterhuene opened PR #2842 from engine-sig-registry to main:

This PR contains a number of internal refactorings that help make Store::register_module do as little work as possible (registration is now only an insertion into a BTreeMap).

This will improve module instantiation performance for modules containing lots of functions and instructions.

The changes are:

• Move the shared signature registry out of Store and into Engine. Modules now register their signatures with the registry upon creation and store the signature index -> shared index mappings internally.
• Engines now register the signatures of any host functions on Config upon engine creation as well as store the anyfunc representations of the host functions (previously kept in Store).
• Removed StoreFrameInfo and StackMapRegistry in favor of a unified ModuleRegistry per-store. The module registry can be queried based on program counter values to quickly locate the owning module.
• With the removal of StackMapRegistry, the runtime is now given a trait object to call back on for finding the stack map for a program counter value during GC. The lookup uses the existing data in CompiledModule which is already optimized for the search.

This PR supersedes #2820.

Wasmtime GitHub notifications bot (Apr 14 2021 at 23:44):

peterhuene requested alexcrichton for a review on PR #2842.

Wasmtime GitHub notifications bot (Apr 14 2021 at 23:44):

peterhuene requested alexcrichton and fitzgen for a review on PR #2842.

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

FWIW I think originally I made this function (or something like it) to prevent access to all the internals of FunctionInfo (or w/e it's called in this module) to ensure that the caller only got access to the items it needed, but if we're returning 3-element tuples we can probably go ahead and just return a reference to the struct itself

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

Having only read this far in the diff, this seems like something that should happen automatically and shouldn't need a destructor. Should this logic be behind a debug_assertions cfg?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

Since this returns *const StackMap this should be an unsafe trait I think?

That being said though it'd be great if this could return &StackMap, it feels sort of unnecessarily unsafe to return a *const pointer here. Since this is a purely internal trait and it's just for our own purposes, could we go ahead and update this to returning a Ref<StackMap> so we can persist the borrow that's happening within wasmtime?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

Could this be preceded with a debug assert that the entry is None?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

This could use get_mut I think instead of write() perhaps?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

Instead of hashing WasmFuncType this could lookup index within the sigs map just created I think?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

I think the original intention was to avoid allocations here, but since lookup_func_type clones the signature this can probably just call self.ty().params().len() perhaps?

(afaik the speed of this function isn't paramount)

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

I'm a bit wary having an unregister function like this mostly because it's related to memory safety in that it's comparing function signatures at the wasm level. This feels like manual memory management which can be error prone and difficult to get right.

Would it be possible, though, to have the two registration functions above return an owned handled which has a destructor? We could store that owned handle within Func/HostFunc/Module/etc ideally and have the destructor automatically take care of the cleanup.

(not sure if that requires bending over backwards too much)

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

Could this return & instead of *const and it's coerced to something unsafe by the caller?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

Since this is a one-liner now could this method get removed in favor of putting the borrow_mut on the caller?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

Could this debug_assert that the start is the same?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

This is one of the things I was a little worried about with the signatures and ownership of the returned shared indices. In the middle of this code it's not clear to me (immediately at least) how this is valid for the lifetime of a RegisteredModule. I figured that the lifetime of indexes for a signature would be tied to a Module itself but the Module isn't persisted here and we just retain a signature map.

This might just be that I don't have the signature management all in my head though. I don't think this is a great place for a comment per se, but I'm curious what you think about this. Also, how does this ensure that the module's signatures' registrations outlive this RegisteredModule?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

FWIW this seems like it could be prohibitively slow or a source of accidental n^2 things. I'm basically worried that this linear search could take a surprisingly long time in situations where a store has a number of modules or something like that (especially with module linking and whatnot).

I think this is only called from Func::from_wasmtime_function so the impact is at least somewhat isolated, but even then that's a pretty core piece of functionality that I'm not entirely sure how would be refactored.

Do you have thoughts on how this might be possible to remove the loop?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

I'm a little wary of this too, there's no need for the global registration to hang on to signatures, right?

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

(I think the way we access info has improved over time so "limiting what you can access" is much less important than I originally thought it might be)

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

If this function is only used for deregistration, could this perhaps return an iterator over the signature index and the reference count? That way the deregistration wouldn't have to do lookup multiple times in a row

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

As module construction gets a little more complicated, could this be outlined in a new wasmtime-only constructor on Module? I'm imagining that ideally this function and Module::from_binary would end up in the same place which si the only place that actually literally constructs a Module

Wasmtime GitHub notifications bot (Apr 15 2021 at 15:22):

alexcrichton created PR Review Comment:

(and also have a comment as to why it's an early-return here for double-registration of a module.

Wasmtime GitHub notifications bot (Apr 15 2021 at 16:40):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 16:40):

peterhuene created PR Review Comment:

I'm fine with changing RegisteredModule to store Module and just rely on that to get at the compiled module and signatures.

I think the original intention was to just keep alive the bare minimum for the registry's operation, as with module linking implemented, there's a bunch of stuff stored in Module geared towards instantiation that could be dropped while the module is still in use by a Store (i.e. no longer instantiatable, but an instance still exists for the module). But if it's confusing ownerships and lifetimes, perhaps it's not worth it.

Wasmtime GitHub notifications bot (Apr 15 2021 at 16:47):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 16:47):

peterhuene created PR Review Comment:

Re: the question about how this ensures signatures outlive the RegisteredModule: only that the module's shared signatures are only unregistered on drop of ModuleSharedSignatures and the Arc references the only owner of that. So for it to drop: the referencing ModuleInner needs to drop and all stores where the module is registered need to drop; at that point the signatures are no longer needed.

Wasmtime GitHub notifications bot (Apr 15 2021 at 16:48):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 15 2021 at 16:58):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 16:58):

peterhuene created PR Review Comment:

If the shared signature space were guaranteed contiguous for a module, we could easily keep a range map around for at least O(log(n)) lookup, but the free list maintained by the signature registry precludes that.

Let me think on this more.

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen created PR Review Comment:

Do these VMCallerCheckedAnyfuncs need to be boxed? The only reason I would assume so is if their addresses are taken and assumed stable. If that is the case, then I would expect some comments here warning readers about that.

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen created PR Review Comment:

Having read a bit more, I guess the SharedSignatures structure sort of is this RAII type, but for a whole collection of signatures.

I do think that this function should still be marked unsafe though, and have a comment laying out why, as discussed up above.

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen created PR Review Comment:

Also, because memory safety relies on these indices being unique for a given signature's lifetime, and unregistering allows reusing the index, then this function should be marked unsafe (and then the RAII type that Alex describes would be the thing that encapsulates this unsafety and internally has an unafe block to make this call).

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen created PR Review Comment:

One final thing: since nothing can guarantee that we aren't currently "borrowing" an index out of the SharedSignatures by the time we drop it and unregister its indices (because the indices aren't references and/or types with a lifetime and they are used by JIT code and such) I think that actually creating a SharedSignatures should be unsafe and have the contract that "by the time you drop this, you promise you aren't 'borrowing' any indices from within it anymore".

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:46):

fitzgen created PR Review Comment:

(And if their addresses aren't being taken and assumed stable, then can we remove the boxing here?)

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:51):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 20:51):

peterhuene created PR Review Comment:

Indeed, that is the intention as Func's ABI representation is *mut VMCallerCheckedAnyfunc. Normally that's stored with the export in Func, but HostFunc is shared across engines and thus the engine needs to be responsible for (stably) storing the anyfunc with the appropriate shared signature index for the engine.

I'll update this with a comment.

Wasmtime GitHub notifications bot (Apr 15 2021 at 22:14):

alexcrichton created PR Review Comment:

Nah I think it's a good idea to not store Module here so we can split it up as necessary.

I think I just got confused a bit as we have so many module wrappers and module bits and pieces I couldn't keep them all straight in my head. I realize now though that the lifetime here is obvious in that this is the point a store retains a module's signatures, and the drop of these signatures is what deregisters.

I think that this is probably fine to leave as-is, I just needed to sit and think and understand more about what was going on

Wasmtime GitHub notifications bot (Apr 15 2021 at 22:14):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 22:18):

alexcrichton created PR Review Comment:

Since this is only called from from_wasmtime_function could we perhaps use the entry point of the wasm function provided there for lookup? If this took a VMCallerCheckedAnyfunc instead of a VMSharedSignatureIndex we could do a pc lookup to find which module has that pc, then we look up in that module's trampoline map directly?

Wasmtime GitHub notifications bot (Apr 15 2021 at 22:18):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 23:17):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 23:17):

peterhuene created PR Review Comment:

OooOoo excellent idea :) I'll make that change.

Wasmtime GitHub notifications bot (Apr 15 2021 at 23:29):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 23:29):

peterhuene created PR Review Comment:

Is there a good way to do this given Ref::filter_map is unstable?

Wasmtime GitHub notifications bot (Apr 15 2021 at 23:31):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 16 2021 at 01:39):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 01:39):

peterhuene created PR Review Comment:

Ok I've reworked this such that Engine, Module, and Store all internally use a SignatureCollection that has a reference on the signature registry. This is an RAII type that will unregister the signatures on drop.

This cleans up the code quite a bit and makes it less error prone. I'll push up the change soon.

Wasmtime GitHub notifications bot (Apr 16 2021 at 01:40):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 16 2021 at 03:20):

peterhuene updated PR #2842 from engine-sig-registry to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:42):

peterhuene updated PR #2842 from engine-sig-registry to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:51):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:51):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:51):

alexcrichton created PR Review Comment:

The leading &* can be removed here I think

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:51):

alexcrichton created PR Review Comment:

This seems to compile for me:

trait A: 'static {}

fn foo(a: Option<*const dyn A>) {}

fn bar(a: &dyn A) {
    foo(Some(a));
}

so is this transmute required? (same for the one in instance.rs)

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:51):

alexcrichton created PR Review Comment:

The leading &* I think can be removed here

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:59):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 17:59):

peterhuene created PR Review Comment:

Adding the 'static constraint to the trait did the trick, so I'll remove the transmute

Wasmtime GitHub notifications bot (Apr 16 2021 at 18:04):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 18:04):

peterhuene created PR Review Comment:

Thanks, forgot to update as I changed stack_map_lookup's return type.

Wasmtime GitHub notifications bot (Apr 16 2021 at 18:14):

peterhuene updated PR #2842 from engine-sig-registry to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 19:07):

peterhuene updated PR #2842 from engine-sig-registry to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 19:25):

peterhuene updated PR #2842 from engine-sig-registry to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 19:31):

peterhuene updated PR #2842 from engine-sig-registry to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 19:31):

peterhuene requested alexcrichton for a review on PR #2842.

Wasmtime GitHub notifications bot (Apr 16 2021 at 19:34):

peterhuene updated PR #2842 from engine-sig-registry to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 19:45):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 20:59):

peterhuene merged PR #2842.

Last updated: Dec 23 2024 at 13:07 UTC