Stream: git-wasmtime

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

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

Wasmtime GitHub notifications bot (Dec 21 2023 at 16:48):

rvolosatovs updated PR #7718.

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:

• 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 (Dec 22 2023 at 13:57):

rvolosatovs updated PR #7718.

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

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?

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:

• Serve as a opt-in simple solution for use cases where initial linking performance is not a bottleneck/not critical and/or implementation simplicity and readability are more important - this is also relevant for prototyping.
• While a more performant solution (hooking into the bindgen, I suppose) is built, unblock WASI resource ResourceAny conversion.
• (in the general case) Provide for use cases where the Linker::resource calls are wrapped in an external API. (in case that Wasmtime WASI implementation is updated to expose the mapping somehow, third parties may still provide APIs wrapping Linker::resource without exposing the indexes and lack of path-based lookups, even if less performant than they could otherwise be, would block users from benefiting from those said APIs)

Wasmtime GitHub notifications bot (Jan 08 2024 at 12:41):

rvolosatovs updated PR #7718.

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:

• Serve as a opt-in simple solution for use cases where initial linking performance is not a bottleneck/not critical and/or implementation simplicity and readability are more important - for example, it's very relevant for rapid prototyping.
• While a more performant solution (hooking into the bindgen, I suppose) is built, unblock WASI resource ResourceAny conversion.
• (in the general case) Provide for use cases where the Linker::resource calls are wrapped in an external API. (in case that Wasmtime WASI implementation is updated to expose the mapping somehow, third parties may still provide APIs wrapping Linker::resource without exposing the indexes and lack of path-based lookups, even if less performant than they could otherwise be, would block users from benefiting from those said APIs)

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:

• Serve as a opt-in simple solution for use cases where initial linking performance is not a bottleneck/not critical and/or implementation simplicity and readability are more important - for example, it's very relevant for rapid prototyping.
• While a more performant solution (hooking into the bindgen, I suppose) is built, unblock WASI resource ResourceAny conversion.
• (in the general case) Provide for use cases where the Linker::resource calls are wrapped in an external API. (in case that Wasmtime WASI implementation is updated to expose the mapping somehow, third parties may still provide APIs wrapping Linker::resource without exposing the indexes and lack of path-based lookups, even if less performant than they could otherwise be, would block users from benefiting from those said APIs)

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)

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:

• Serve as a opt-in simple solution for use cases where initial linking performance is not a bottleneck/not critical and/or implementation simplicity and readability are more important - for example, it's very relevant for rapid prototyping.
• While a more performant solution (hooking into the bindgen, I suppose) is built, unblock WASI resource ResourceAny conversion.
• (in the general case) Provide for use cases where the Linker::resource calls are wrapped in an external API. (in case that Wasmtime WASI implementation is updated to expose the mapping somehow, third parties may still provide APIs wrapping Linker::resource without exposing the indexes and lack of path-based lookups, even if less performant than they could otherwise be, would block users from benefiting from those said APIs)

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

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:

• Serve as a opt-in simple solution for use cases where initial linking performance is not a bottleneck/not critical and/or implementation simplicity and readability are more important - for example, it's very relevant for rapid prototyping.
• While a more performant solution (hooking into the bindgen, I suppose) is built, unblock WASI resource ResourceAny conversion. (#7714)
• (in the general case) Provide for use cases where the Linker::resource calls are wrapped in an external API. (in case that Wasmtime WASI implementation is updated to expose the mapping somehow, third parties may still provide APIs wrapping Linker::resource without exposing the indexes and lack of path-based lookups, even if less performant than they could otherwise be, would block users from benefiting from those said APIs)

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

Wasmtime GitHub notifications bot (Jan 08 2024 at 12:50):

rvolosatovs has marked PR #7718 as ready for review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 12:50):

rvolosatovs requested fitzgen for a review on PR #7718.

Wasmtime GitHub notifications bot (Jan 08 2024 at 12:50):

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

Wasmtime GitHub notifications bot (Jan 08 2024 at 12:52):

rvolosatovs edited PR #7718:

Follow-up on #7688
Closes #7714

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)

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)

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:

• Currently RuntimeImportIndex is a "private" type which is just an implementation detail of Wasmtime. This is exporting it to the public interface without explaining what it means or represents. Under the hood it's sort of an irrelevant implementation detail to embedders where it happens to be the specific order of arguments being passed to the component and it can be used to lookup the runtime type used for a particular import.
• Additionally the concept of a "path" for an import is introduced here in the public API. The Linker for example does not operate on this and instead works with instances.

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: Nov 22 2024 at 16:03 UTC