wasmtime / PR #7804 feat: support component instance impo... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7804 feat: support component instance impo...

Wasmtime GitHub notifications bot (Jan 22 2024 at 17:46):

rvolosatovs opened PR #7804 from rvolosatovs:feat/component-import-introspection to bytecodealliance:main:

This allows for embedders to introspect the component instance imports 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

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:08):

alexcrichton submitted PR review:

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.

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:48):

rvolosatovs updated PR #7804.

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:49):

rvolosatovs commented on PR #7804:

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 https://github.com/bytecodealliance/wasmtime/pull/7804/commits/56f68d578a74694eb31e2cbc67ec4a913fb0b113 ?
Otherwise, happy to jump on a call tomorrow

Wasmtime GitHub notifications bot (Jan 22 2024 at 19:52):

alexcrichton commented on PR #7804:

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

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Could this perhaps return a types::Component? If so then let's call this method substituted_component_type where the documentation indicates that the resource types imported by the component specified as an argument are replaced with the imports present in Linker in the returned type.

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Could those returning a CoreFunc return a FuncType directly instead?

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Since types is a public module it's ok to avoid these reexports to avoid the same type being known under two different names.

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Could this and results be ExactSizeIterator?

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Like above, ExactSizeIterator + &str

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Could &(String, String), .. here become &str, &str, ...

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Could this get the ExactSizeIterator plus &str treatment? (and exports below too)

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Could this be removed in favor of using ResourceType directly?

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Could this be &str instead of &String?

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

Implementing this would probably require adding a ComponentTypeIndex somewhere inside of a component since I don't think that exists today. Furthermore that may not be altogether trivial to add.

If it's not easy to add, some suggestions on this method:

ExactSizeIterator + &str return value in the iterator

Calling this substituted_import_types and documenting that the Linker's resources are substituted in the component types.

Adding a substituted_export_types here as well for symmetry

Wasmtime GitHub notifications bot (Jan 22 2024 at 21:05):

alexcrichton created PR review comment:

also ExactSizeIterator for this and exports

Wasmtime GitHub notifications bot (Jan 22 2024 at 22:44):

github-actions[bot] commented on PR #7804:

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 (Jan 23 2024 at 20:22):

rvolosatovs updated PR #7804.

Last updated: Nov 22 2024 at 16:03 UTC