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 component type introspection #7804

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Jan 22, 2024

This allows for embedders to introspect the component type to e.g. construct Vals of complex types expected by the components at runtime.
Closes #7726
I'll work on adding a simple test once I verify this covers my own use case

cc @alexcrichton

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 for this! At a high level this looks pretty good, although there's two parts of this I'd have a comment on. One is that from a component model point of view an instance does not have imports, only exports and their types. In that sense Instance::import_types is not necessarily a component-model-based primitive here, and that's where Component::imports() would show up instead. I suspect though that this is critical to your use case, though.

To handle this I might recommend a slightly updated approach. From a Linker it should be possible to acquire a types::Component from a component::Component. This would synthesize the "InstanceType" data internally required to get a Handle which is more-or-less a glorified resource mapping. That would be a built-in way of asking "If I instantiate this component using this linker what resource would correspond to this import?" (happy to jump on a call to explain this more too)

The second thing is mostly minor API adjustments here and there, but nothing requiring major refactoring, so I'll note those later.

@rvolosatovs
Copy link
Member Author

Thanks for this! At a high level this looks pretty good, although there's two parts of this I'd have a comment on. One is that from a component model point of view an instance does not have imports, only exports and their types. In that sense Instance::import_types is not necessarily a component-model-based primitive here, and that's where Component::imports() would show up instead. I suspect though that this is critical to your use case, though.

To handle this I might recommend a slightly updated approach. From a Linker it should be possible to acquire a types::Component from a component::Component. This would synthesize the "InstanceType" data internally required to get a Handle which is more-or-less a glorified resource mapping. That would be a built-in way of asking "If I instantiate this component using this linker what resource would correspond to this import?" (happy to jump on a call to explain this more too)

The second thing is mostly minor API adjustments here and there, but nothing requiring major refactoring, so I'll note those later.

Thanks for the quick response, is this what you mean 56f68d5 ?
Otherwise, happy to jump on a call tomorrow

@alexcrichton
Copy link
Member

Yep exactly!

I'll go over this with a more fine-toothed comb tonight so you can have a review for when you wake up

crates/wasmtime/src/component/linker.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/mod.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/types.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 22, 2024
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.

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>
…s/results

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/component-import-introspection branch from 56f68d5 to d48e9f3 Compare January 23, 2024 20:22
@rvolosatovs rvolosatovs changed the title feat: support component instance import type introspection feat: support component type introspection Jan 23, 2024
@rvolosatovs
Copy link
Member Author

rvolosatovs commented Jan 23, 2024

@alexcrichton I've studied the wasmtime_environ implementation a little bit and looks like I came up with a solution that makes sense (after all types are resolved, "synthesize" a TypeComponent and store in ComponentTypes on finish to acquire a type index, which is then propagated further).
I had to also add the type index info to exports, which was missing.

I've added a test case reusing a big chunk of the dynamic::everything component (it does not actually contain everything, but hopefully this is "good enough"). PTAL

@rvolosatovs rvolosatovs marked this pull request as ready for review January 23, 2024 20:28
@rvolosatovs rvolosatovs requested a review from a team as a code owner January 23, 2024 20:28
@rvolosatovs rvolosatovs requested review from alexcrichton and removed request for a team January 23, 2024 20:28
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/component-import-introspection branch from 7b22dc9 to a1760af Compare January 25, 2024 12:39
@rvolosatovs
Copy link
Member Author

rvolosatovs commented Jan 25, 2024

@alexcrichton although in our conversation yesterday we discussed that the static module and "instance item bag" case type indexes should be constructed somewhere along the way and thrown out, that did not seem to be the case in all occurrences, therefore in the proposed approach the static module type is "synthesized" from compiled artifacts on finish for cases where instance type is not assigned yet, that is also done recursively in the same step.
Note the updated test case - I duplicated, for the most part, test cases that were causing panics during development due to type not being available (in particular, the "instance item bag" case)

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/component-import-introspection branch from a1760af to 76c827a Compare January 25, 2024 12:49
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.

Looks great to me, thanks!

@alexcrichton alexcrichton added this pull request to the merge queue Jan 26, 2024
Merged via the queue into bytecodealliance:main with commit 2288538 Jan 26, 2024
18 checks passed
@rvolosatovs rvolosatovs deleted the feat/component-import-introspection branch January 26, 2024 20:49
@sehz
Copy link

sehz commented Jan 28, 2024

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow component::LinkerInstance::func_new functions to return complex types
3 participants