Stream: git-wasmtime

Topic: wasmtime / PR #7718 feat: support `RuntimeImportIndex` lo...


view this post on Zulip Wasmtime GitHub notifications bot (Dec 21 2023 at 16:32):

rvolosatovs opened PR #7718 from rvolosatovs:feat/dynamic-resources-by-path to bytecodealliance:main:

Follow-up on #7688
Closes #7714

view this post on Zulip Wasmtime GitHub notifications bot (Dec 21 2023 at 16:48):

rvolosatovs updated PR #7718.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 21 2023 at 19:44):

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

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "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 (Dec 22 2023 at 13:57):

rvolosatovs updated PR #7718.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 22 2023 at 13:59):

rvolosatovs edited PR #7718:

Follow-up on #7688
Closes #7714

Just https://github.com/bytecodealliance/wasmtime/pull/7718/commits/3680ff4c32d06fba7c43e27ab2030bf32927b116 relevant here

view this post on Zulip Wasmtime GitHub notifications bot (Jan 02 2024 at 20:58):

alexcrichton commented on PR #7718:

I personally fall on the conservative side of API design, so my knee-jerk reaction is to not include something like this. Could you expand a bit more on the motivation, however?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:40):

rvolosatovs commented on PR #7718:

I personally fall on the conservative side of API design, so my knee-jerk reaction is to not include something like this. Could you expand a bit more on the motivation, however?

Sure, this is primarily to address https://github.com/bytecodealliance/wasmtime/issues/7714 and potentially help with https://github.com/bytecodealliance/wasmtime/issues/7726.
Most important reason for this to exist is that the embedder may not necessarily be directly calling e.g. Linker::resource and therefore may not have the ResourceImportIndex associated with it - this is the case today, for example, for any resources added to the linker via generated add_to_linker from bindgen. To address this add_to_linker would need to return a mapping of resource type import paths (expressed as strings, generated statically-typed structs or otherwise) to their respective import indexes. The intention of this change is therefore to:

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:41):

rvolosatovs updated PR #7718.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:42):

rvolosatovs edited a comment on PR #7718:

I personally fall on the conservative side of API design, so my knee-jerk reaction is to not include something like this. Could you expand a bit more on the motivation, however?

Sure, this is primarily to address https://github.com/bytecodealliance/wasmtime/issues/7714 and potentially help with https://github.com/bytecodealliance/wasmtime/issues/7726.
Most important reason for this to exist is that the embedder may not necessarily be directly calling e.g. Linker::resource and therefore may not have the ResourceImportIndex associated with it - this is the case today, for example, for any resources added to the linker via generated add_to_linker from bindgen. To address this add_to_linker would need to return a mapping of resource type import paths (expressed as strings, generated statically-typed structs or otherwise) to their respective import indexes. The intention of this change is therefore to:

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:46):

rvolosatovs edited a comment on PR #7718:

I personally fall on the conservative side of API design, so my knee-jerk reaction is to not include something like this. Could you expand a bit more on the motivation, however?

Sure, this is primarily to address https://github.com/bytecodealliance/wasmtime/issues/7714 and potentially help with https://github.com/bytecodealliance/wasmtime/issues/7726.
Most important reason for this to exist is that the embedder may not necessarily be directly calling e.g. Linker::resource and therefore may not have the ResourceImportIndex associated with it - this is the case today, for example, for any resources added to the linker via generated add_to_linker from bindgen. To address this add_to_linker would need to return a mapping of resource type import paths (expressed as strings, generated statically-typed structs or otherwise) to their respective import indexes. The intention of this change is therefore to:

Short term, by far the most important aspect of this for me is unblocking the "WASI resource -> ResourceAny"conversion (#7714) without the need for manual reimplementation of add_to_linker for all interfaces containing resources. Perhaps you have another idea how this can be solved?

Note that one potential compromise here, which could assist with the string lookup performance penalty, could be introducing a "cell" type, which could store the initial lookup result (originally proposed by @fitzgen in a very brief chat about this topic)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:48):

rvolosatovs edited a comment on PR #7718:

I personally fall on the conservative side of API design, so my knee-jerk reaction is to not include something like this. Could you expand a bit more on the motivation, however?

Sure, this is primarily to address https://github.com/bytecodealliance/wasmtime/issues/7714 and potentially help with https://github.com/bytecodealliance/wasmtime/issues/7726.
Most important reason for this to exist is that the embedder may not necessarily be directly calling e.g. Linker::resource and therefore may not have the ResourceImportIndex associated with it - this is the case today, for example, for any resources added to the linker via generated add_to_linker from bindgen. To address this add_to_linker would need to return a mapping of resource type import paths (expressed as strings, generated statically-typed structs or otherwise) to their respective import indexes. The intention of this change is therefore to:

Short term, by far the most important aspect of this for me is unblocking the "WASI resource -> ResourceAny"conversion (#7714) without the need for manual reimplementation of add_to_linker for all interfaces containing resources. Perhaps you have another idea how this can be solved?

Note that one potential compromise here, which could assist with the string lookup performance penalty, could be introducing a "cell" type, which could store the initial lookup result (originally proposed by @fitzgen in a very brief chat about this topic)

I suspect this we will end up in a very similar situation here working on #7726

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:49):

rvolosatovs edited a comment on PR #7718:

I personally fall on the conservative side of API design, so my knee-jerk reaction is to not include something like this. Could you expand a bit more on the motivation, however?

Sure, this is primarily to address https://github.com/bytecodealliance/wasmtime/issues/7714 and potentially help with https://github.com/bytecodealliance/wasmtime/issues/7726.
Most important reason for this to exist is that the embedder may not necessarily be directly calling e.g. Linker::resource and therefore may not have the ResourceImportIndex associated with it - this is the case today, for example, for any resources added to the linker via generated add_to_linker from bindgen. To address this add_to_linker would need to return a mapping of resource type import paths (expressed as strings, generated statically-typed structs or otherwise) to their respective import indexes. The intention of this change is therefore to:

Short term, by far the most important aspect of this for me is unblocking the "WASI resource -> ResourceAny"conversion (#7714) without the need for manual reimplementation of add_to_linker for all interfaces containing resources. Perhaps you have another idea how this can be solved?

Note that one potential compromise here, which could assist with the string lookup performance penalty, could be introducing a "cell" type, which could store the initial lookup result (originally proposed by @fitzgen in a very brief chat about this topic)

I suspect this we will end up in a very similar situation here working on #7726

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:50):

rvolosatovs has marked PR #7718 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:50):

rvolosatovs requested fitzgen for a review on PR #7718.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:50):

rvolosatovs requested wasmtime-core-reviewers for a review on PR #7718.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:52):

rvolosatovs edited PR #7718:

Follow-up on #7688
Closes #7714

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:53):

rvolosatovs edited PR #7718:

Follow-up on #7688
Closes #7714 (a more performant solution should probably exist, but this addressed the immediate issue)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 12:53):

rvolosatovs edited PR #7718:

Follow-up on #7688
Closes #7714 (a more performant solution should probably exist, but this addresses the immediate issue)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 08 2024 at 16:22):

alexcrichton commented on PR #7718:

Thanks for expanding! What you're saying makes sense and give me a better understanding as to the pieces here.

I'd be inclined to go the route of #7714 first, however. This PR is exposing yet-more-details about the internal representation of components which otherwise needs documentation (e.g. the doc blocks here don't really explain anything about the deeper meaning of the constructs here). For example:

I understand that this is an "easy" PR but that's typically how these things go. Wasmtime doesn't implement the real feature here (#7714) so it's easier to basically make more things pub than to implement the real feature. The cost of this though is maintenance over time. These APIs will need to be documented and maintained and kept in sync with all other component-related APIs. Additionally the runtime functionality would need to be preserved here across refactorings of Wasmtime's internals and all that.

More-or-less I understand that this is an easy PR and gets the job done, but if you're ok I'd prefer to go down the route of #7714 first to avoid adding two more concepts to the component embedding API (runtime import indices and import paths). If it's necessary to add them then it's necessary but if possible I'd prefer to avoid adding them since the embedding API is already quite complicated.


Last updated: Dec 23 2024 at 12:05 UTC