-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: main
Are you sure you want to change the base?
feat: support RuntimeImportIndex
lookups by string paths
#7718
Conversation
28cb7fd
to
b3ddcf4
Compare
Subscribe to Label Actioncc @peterhuene
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 |
b3ddcf4
to
3680ff4
Compare
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>
Sure, this is primarily to address #7714 and potentially help with #7726.
Short term, by far the most important aspect of this for me is unblocking the "WASI resource -> 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 |
3680ff4
to
d7d96d1
Compare
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 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. |
Follow-up on #7688
Closes #7714 (a more performant solution should probably exist, but this addresses the immediate issue)