Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support RuntimeImportIndex lookups by string paths #7718

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Dec 21, 2023

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

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Dec 21, 2023
Copy link

Subscribe to Label Action

cc @peterhuene

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

Learn more.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Dec 22, 2023
@alexcrichton
Copy link
Member

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?

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
Copy link
Member Author

rvolosatovs commented Jan 8, 2024

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 #7714 and potentially help with #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. (Support Resource<T> -> ResourceAny for WASI resources #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

@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources-by-path branch from 3680ff4 to d7d96d1 Compare January 8, 2024 12:41
@rvolosatovs rvolosatovs marked this pull request as ready for review January 8, 2024 12:50
@rvolosatovs rvolosatovs requested a review from a team as a code owner January 8, 2024 12:50
@rvolosatovs rvolosatovs requested review from fitzgen and removed request for a team January 8, 2024 12:50
@alexcrichton
Copy link
Member

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.

@rvolosatovs rvolosatovs marked this pull request as draft January 8, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Resource<T> -> ResourceAny for WASI resources
2 participants