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: implement Resource<T> -> ResourceAny conversion #7688

Merged

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Dec 14, 2023

Closes #7676
Follow-up on #7680

This feature requires changes in the resource table to allow specifying the ResourceType associated with the resource stored (since assumption that it's always equal to ResourceType::host::<T>() is no longer valid)

Add support for Resource<T> -> ResourceAny conversion to allow invoking component functions without bindings generated at compile-time.

@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources branch from 6269937 to ae2d9c7 Compare December 14, 2023 14:01
@alexcrichton
Copy link
Member

I think I might have missed something in this by accident perhaps. I don't think that this is the direction things should go in, for example Resource<T> is designed to have a statically known type as ResourceType::host::<T>() as opposed to a runtime-defined type. Having a runtime-defined type sort of defeats the purpose of the type parameter which at that point require a redesign of the resource-related APIs.

The title of this PR though, allowing host creation of guest resources, is not something Wasmtime can enable. Just as guests can't create host-defined resources without asking the host to do so the other way around is the same. The host can't conjure a guest resource out of thin air, it must originate in the guest.

Given that I fear I might have missed some motivation/use case when talking about this, could you perhaps clarify?

@rvolosatovs
Copy link
Member Author

rvolosatovs commented Dec 14, 2023

Given that I fear I might have missed some motivation/use case when talking about this, could you perhaps clarify?

Here's a more concrete example of the use case:

Assume the following WIT:

interface types {
    use wasi:io/poll@0.2.0-rc-2023-11-10.{pollable};

    resource future-string {
        subscribe: func() -> pollable;
        get: func() -> option<string>;
    }
}

interface example {
    use types.{future-string};

    foo: func(opaque: future-string);
}

world opaque-world {
    export example;
}

And a component exporting the example interface.

The intention is to call example.foo on the component without having the WIT at compilation time, i.e. parsing the WIT from the Wasm itself at runtime (which works already) and then invoking example.foo within the component.
The only blocker for this now is the fact that the host cannot construct future-string at runtime if it does not have the future-string WIT definition available at compilation time (and associated bindings)

The host, however, can already push a Resource to the table and link the associated methods on the linker, the only bit left is being able to turn the Resource constructed at runtime into a ResourceAny to be able to invoke the function via call or call_async

@rvolosatovs
Copy link
Member Author

rvolosatovs commented Dec 14, 2023

Another way to look at this would be the following:

  • If one could pass a wasmtime::component::Val::U32, which stores the Resource::rep value as a function parameter directly where a resource is expected, that would cover the use case.

Perhaps an even more generic view on this could be allowing arbitrary impl Lower values to be passed as arguments to call/call_async (or yet another alternative)

This all really boils down to being able to pass the resource index to a component in cases where the resource type is not known at compilation time

@github-actions github-actions bot added wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself labels Dec 14, 2023
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasi", "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.

@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources branch from ae2d9c7 to 32ef4ea Compare December 15, 2023 15:02
@rvolosatovs rvolosatovs changed the title feat: allow construction of guest-defined resources feat: implement Resource<T> -> ResourceAny conversion Dec 15, 2023
@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources branch from 32ef4ea to d1c0142 Compare December 15, 2023 16:49
@rvolosatovs
Copy link
Member Author

Small update: I found a way to obtain the TypeResourceTableIndex from the instance, which simplifies getting the destructor and flags, however I keep running into

    type mismatch for own, possibly due to mixing distinct composite types

I will reach-out offline

@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources branch 6 times, most recently from 7e9054f to a212201 Compare December 18, 2023 14:06
@rvolosatovs rvolosatovs marked this pull request as ready for review December 18, 2023 14:24
@rvolosatovs rvolosatovs requested a review from a team as a code owner December 18, 2023 14:24
@rvolosatovs rvolosatovs requested review from fitzgen and removed request for a team December 18, 2023 14:24
@rvolosatovs
Copy link
Member Author

@alexcrichton , as discussed, I introduced a PathIndex mapping usizes to Vec<usize> import paths.
I did not attempt to work on optimizing this, but rather focused on getting something simple over the line.
I added the PathIndex to the NameMap value, since in order to use these indexes a reverse mapping from Vec<usize> import path to PathIndex is required

crates/wasmtime/src/component/linker.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/linker.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/resources.rs Outdated Show resolved Hide resolved
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for this! A few comments which I'm happy to defer to later, but for the implementation of LinkerInstance::resource I think it'd be good to continue to rely on the insert helper. I wouldn't worry too much about the interaction of bumping the resource counter if an error is returned.

crates/wasmtime/src/component/instance.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/linker.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/linker.rs Show resolved Hide resolved
@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources branch from ab6d971 to 23d5411 Compare December 22, 2023 13:44
@rvolosatovs rvolosatovs requested a review from a team as a code owner December 22, 2023 13:44
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Dec 22, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Jan 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 2, 2024
@elliottt elliottt removed the request for review from a team January 2, 2024 21:51
@sehz
Copy link

sehz commented Jan 3, 2024

Any progress? This is blocker for us

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources branch 2 times, most recently from 06f11f2 to 3af16f3 Compare January 3, 2024 20:54
@rvolosatovs
Copy link
Member Author

Any progress? This is blocker for us

Hey @sehz,

There was a doc CI failure:

error: unresolved link to Linker::resource
--> crates/wasmtime/src/component/resources.rs:519:60
|
519 | /// idx is the [ResourceImportIndex] returned by [Linker::resource].
| ^^^^^^^^^^^^^^^^ no item named Linker in scope

warning: cranelift-jit (lib doc) generated 1 warning (1 duplicate)
Documenting wasmtime-cranelift-shared v17.0.0 (/home/runner/work/wasmtime/wasmtime/crates/cranelift-shared)
warning: wasmtime (lib doc) generated 1 warning (1 duplicate)
error: could not document wasmtime

Which, I believe, is now fixed. I've rebased and added prtest:full(not sure if a scoped option exists) to run all CI checks on this PR, once that succeeds it should be ready for merge.

Please note that I'm generally out of office until Monday, the 8th of January, but I may be able to address small things like this doc CI failure in the meantime

prtest:full

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/dynamic-resources branch from 3af16f3 to 17923ff Compare January 3, 2024 21:42
@alexcrichton alexcrichton added this pull request to the merge queue Jan 3, 2024
Merged via the queue into bytecodealliance:main with commit ae5d284 Jan 3, 2024
40 checks passed
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 wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime host resource types
3 participants