wasmtime / PR #7892 Refactor `wasmtime::FuncType` to hold... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7892 Refactor `wasmtime::FuncType` to hold...

Wasmtime GitHub notifications bot (Feb 07 2024 at 23:41):

fitzgen requested alexcrichton for a review on PR #7892.

Wasmtime GitHub notifications bot (Feb 07 2024 at 23:41):

fitzgen requested wasmtime-core-reviewers for a review on PR #7892.

Wasmtime GitHub notifications bot (Feb 07 2024 at 23:41):

fitzgen opened PR #7892 from fitzgen:make-func-type-cheap to bytecodealliance:main:

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.

Wasmtime GitHub notifications bot (Feb 08 2024 at 01:03):

github-actions[bot] commented on PR #7892:

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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 09 2024 at 11:36):

alexcrichton submitted PR review:

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

Wasmtime GitHub notifications bot (Feb 09 2024 at 11:36):

alexcrichton submitted PR review:

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

Wasmtime GitHub notifications bot (Feb 09 2024 at 11:36):

alexcrichton created PR review comment:

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.

Wasmtime GitHub notifications bot (Feb 09 2024 at 11:36):

alexcrichton created PR review comment:

To confirm, RegisteredType is replacing this right? (IIRC we thought this didn't happen but looks like it does, but you wanted to refactor to make it more clear?)

Wasmtime GitHub notifications bot (Feb 09 2024 at 11:36):

alexcrichton created PR review comment:

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.

Wasmtime GitHub notifications bot (Feb 09 2024 at 16:16):

fitzgen commented on PR #7892:

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.

Wasmtime GitHub notifications bot (Feb 09 2024 at 16:20):

fitzgen commented on PR #7892:

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.

Wasmtime GitHub notifications bot (Feb 09 2024 at 16:21):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 16:21):

fitzgen created PR review comment:

Correct and correct.

I think the clean up logic is a lot more clear now!

Wasmtime GitHub notifications bot (Feb 09 2024 at 17:02):

fitzgen commented on PR #7892:

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

Wasmtime GitHub notifications bot (Feb 09 2024 at 17:36):

alexcrichton commented on PR #7892:

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.

Wasmtime GitHub notifications bot (Feb 09 2024 at 17:50):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 17:50):

fitzgen created PR review comment:

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?

Wasmtime GitHub notifications bot (Feb 09 2024 at 18:38):

fitzgen updated PR #7892.

Wasmtime GitHub notifications bot (Feb 09 2024 at 18:38):

fitzgen requested wasmtime-fuzz-reviewers for a review on PR #7892.

Wasmtime GitHub notifications bot (Feb 09 2024 at 18:40):

fitzgen updated PR #7892.

Wasmtime GitHub notifications bot (Feb 09 2024 at 18:43):

fitzgen updated PR #7892.

Wasmtime GitHub notifications bot (Feb 09 2024 at 18:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 20:40):

fitzgen updated PR #7892.

Wasmtime GitHub notifications bot (Feb 09 2024 at 20:40):

fitzgen requested wasmtime-default-reviewers for a review on PR #7892.

Wasmtime GitHub notifications bot (Feb 09 2024 at 20:41):

fitzgen has enabled auto merge for PR #7892.

Wasmtime GitHub notifications bot (Feb 09 2024 at 21:35):

fitzgen merged PR #7892.

Last updated: Nov 22 2024 at 16:03 UTC