Stream: git-wasmtime

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


view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 23:41):

fitzgen requested alexcrichton for a review on PR #7892.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 23:41):

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

view this post on Zulip 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

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.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip 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:

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

Learn more.
</details>

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip 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:

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

view this post on Zulip 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?)

view this post on Zulip 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.

view this post on Zulip 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:

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.

view this post on Zulip 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:

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

Again, this is something for future exploration, IMO.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 16:21):

fitzgen submitted PR review.

view this post on Zulip 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!

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 17:50):

fitzgen submitted PR review.

view this post on Zulip 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 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?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 18:38):

fitzgen updated PR #7892.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 18:38):

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 18:40):

fitzgen updated PR #7892.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 18:43):

fitzgen updated PR #7892.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 18:52):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 20:40):

fitzgen updated PR #7892.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 20:40):

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 20:41):

fitzgen has enabled auto merge for PR #7892.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 21:35):

fitzgen merged PR #7892.


Last updated: Dec 23 2024 at 12:05 UTC