Refactor wasmtime::FuncType to hold a handle to its registered type

[fitzgen]

Rather than holding a copy of the type directly, it now holds a RegisteredType which internally is

• A VMSharedTypeIndex pointing into the engine's types registry.
• An Arc handle to the engine's type registry.
• An Arc handle to the actual type.

The last exists only to keep it so that accessing a wasmtime::FuncType's parameters and results fast, avoiding any new locking on call hot paths.

This is helping set the stage for further types and TypeRegistry refactors needed for Wasm GC.

[github-actions]

Subscribe to Label Action

cc @fitzgen, @peterhuene

Details

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

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

• fitzgen: fuzzing
• peterhuene: wasmtime:api

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

Learn more.

[alexcrichton]

I'm a little wary in how the type-related stuff feels "heavyweight" with lots of Arc, but I don't know of what else to do and we don't necessarily have a benchmark/threshold that this would or wouldn't become a problem at, so seems fine to stay the course and we can improve later if necessary.

I'm also a little worried about passing in &Engine when reading types since that feels like it can get unwieldy/unergonomic pretty quickly. One solution would be to store Engine in more places but then that also feeds back into the above where types become more heavyweight with more Arcs. I don't feel I have a great sense of balance between these concerns right now.

---

Otherwise though I'd recommend prtest:full for this PR. I know the C API is going to need changes, and substantive ones too. The Wasm C API header file does not pass an engine in when creating a function type, for example, so the C function type is going to need to become different than wasmtime::FuncType

[alexcrichton]

This seems like a method we should be ideally removing through this refactoring (or enabling removing).

The callers I see are:

• ExternType::from_wasmtime - this seems like it should not be necessary since the signature is guaranteed to aleady be registered through the module already so with a bit more plumbing we should be able to convert to a shared index in theory.
• ComponentItem::from - this is I think the same as the above just with components instead of modules
• FuncType::new - this seems justified, but in the sense that this method would get inlined into FuncType::new

It's ok to defer this to later, but wanted to note this down.

[fitzgen]

The first two should still re-register the type (ie increment its ref count), even though it should already be registered, because we wouldn't want to end up in the situation where

• we load a module, registering its types
• get a registered type from the module
• drop the module, unregistering its types
• and now have a dangling function type

We could have a helper to get an already-registered type and increment its ref count, but I'm not sure how that would be any more efficient than what exists now. I guess it could at minimum assert that the type is already registered, but this isn't really giving us much I don't think.

Or maybe I am completely missing your point?

[alexcrichton]

I think this is a bit unfortunate and ideally we'd wrap this via some other means, for example storing a Component in Handle<T> or an Engine in there or something like that. That being said this isn't the end of the world so I think it's reasonable to keep it around for now and see if we can figure out something better in the future.

[fitzgen]

I'm a little wary in how the type-related stuff feels "heavyweight" with lots of Arc

We could make it a single Arc if we moved all of OccupiedEntry into RegisteredType and had an Arc<RegisteredType> instead of an Arc<WasmFuncType>. Most importantly, the refcount would be in the RegisteredType and accessible without the whole registry. Then dropping and cloning a RegisteredType wouldn't take a lock at all, it would just decrement its reference count. If the reference count reached zero, then we would either:

• Leave it at zero and not actualy remove the entry yet, instead relying on some subsequent operation on the registry doing a "sweep" for zero-refcount entries at some point in time, like the next time we insert into the registry. A bit hand-wavy, and does leave us with some floating garbage.

• Have the RegisteredType hold a weak reference to the to whole type registry, and then only take the lock to remove the entry once the refcount reaches zero. Actual ref counting operations that don't reach zero still wouldn't need the lock. This does mean that we would add back another smart pointer though.

I'm happy to explore either option, or even something else, if you can think of anything. But I think it would be best to do so in a follow up PR.

[fitzgen]

One solution would be to store Engine in more places but then that also feeds back into the above where types become more heavyweight with more Arcs.

Another thing I was thinking that might make sense, but I haven't fully explored yet:

• Stop putting the type registry itself in an Arc, just store it in the Engine directly.
• Anything that had an arc of the registry, now has an Engine.
• We can make more things hold an Engine as necessary.

I don't think this would be a magic solution, but I think it would at least make more clear the relationship between an engine and a registry and make it so that if you need access to both things you don't need to choose between

• holding two arcs, or
• an extra indirection to access the registry.

Again, this is something for future exploration, IMO.

[fitzgen]

FWIW, filed WebAssembly/wasm-c-api#190 so that we can eventually implement the relevant bits of the Wasm C API again.

[alexcrichton]

Yeah I think it's reasonable to explore this all in the future, I don't have any great ideas myself so it's mostly otherwise about is this semantically what we want which I believe is "yes", and then overhead/perf needs a guiding use case to help inform it.

Also IMO we need to support the C API as-is while it hasn't changed, so it means that wasm_functype_t will need to buffer-up the arguments and then when it first gets "connectd to" something with an engine that's where FuncType::new would happen.