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

Redesign how component exports work #8786

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

alexcrichton
Copy link
Member

This PR is a series of commits that refactor and reimplement how component exports are modeled in Wasmtime at the API layer. The motivations for this commit are:

  • Currently looking up the exports of a component unconditionally requires string lookups at runtime. Unlike with core modules this can't be frontloaded to a pre-instantiation step. While not a huge amount of runtime it's a problem I've wanted to solve historically.
  • Currently WASI semver compatibility should apply to exports #8395 is unsolved and will require adding even more work to the export lookup path. This is something I've been hesitant to do without implement the previously mentioned optimization as it would slow down the instantiation process even more since now it'd do semver parsing and such.
  • Eventually I'd like to be able to, at the API layer of Wasmtime, reason about the exports of pre-instantiated components to build up a graph of instantiations via InstancePre to enable components to call each other. That isn't done in this PR but this is intended to lay some groundwork for that.

In the process of solving these motivations I've redesigned how exports work on components. Previously a Component only had component_type() and an Instance has a "walker"-style API which could be used to walk through the exports of a component (performing string lookups along the way). All Instance-based infrastructure has now been removed and replaced with ComponentExportIndex.

A ComponentExportIndex represents a preloaded pointer to an export, or where one will be. This can be acquired through Component::export_index which can be done during the compilation/loading process of a component. To handle the nested nature of component instance exports this function also takes an Option<&ComponentExportIndex> representing the parent instance where None is the root of the component. This enables walking the entire exported API of a component with only this function and no need for a nested traversal with types.

Additionally all Instance::get_* methods are generalized to take a new generic argument instead of just a &str. If this argument is a string it's a convenience method to load from the root of the component, but otherwise a ComponentExportIndex can also be passed here. A newInstance::get_export method can be used to lookup ComponentExportIndex at runtime and traverse through the instance nesting as well (and also takes an Option<&ComponentExportIndex> argument for the parent in the same way as Component::export_index).

This all then leads to changes in the output of bindgen!. Namely I wanted to take advantage of these new features so the creation of bindgen-generated wrappers is now done with:

  • A *Pre structure is created from InstancePre<T> and wraps InstancePre<T> as well as each of the ComponentExportIndex values for all the internal functions.
  • A FooPre::instantiate method exists to create a Foo where all functions are looked up via index.
  • A Foo::instantiate convenience is still provided to do these two steps internally.

The diff here is a bit scarier than the actual size of this PR due to the number of updates to the bindgen-generated output for the wit-bindgen tests. Otherwise though much of this PR is refactoring to handle all the above consequences.

This commit flattens the representation of exports in a component to
make them more easily indexable without forcing traversal through the
hierarchy of instance imports/exports to get there.
Don't have it optional in some cases and present in others, instead
ensure there's type information for all component exports immediately
available.
This commit is a change to Wasmtime's public API for
`wasmtime::component::Instance` that reorganizes how component exports
are loaded. Previously there was a system where `Instance::exports()`
was called that that was sort of "iterated over" in a builder-style
pattern to acquire the actual export desired. This required lifetime
trickery for nested instances and some unfortunate API bloat. The major
downside of this approach is that it requires unconditional string
lookups at runtime for exports and additionally does not serve as a
great place to implement the semver-compatible logic of bytecodealliance#8395. The goal
of this refactoring is to pave the way to improving this.

The new APIs for loading exports now look a bit more similar to what's
available for core modules. Notably there's a new
`Component::export_index` method which enables performing a string
lookup and returning an index. This index can in turn be passed to
`Instance::get_*` to skip the string lookup when exports are loaded. The
`Instance::exports` API is then entirely removed and dismantled.

The only piece remaining is the ability to load nested exports which is
done through an `Option` parameter to `Component::export_index`. The
way to load a nested instance is now to first lookup the instance with
`None` as this parameter an then the instance itself is `Some` to look
up an export of that instance. This removes the need for a
recursive-style lifetime-juggling API from wasmtime and in theory helps
simplify the usage of loading exports.
This commit updates the output of `bindgen!` to have a different setup
for exports of worlds to handle the changes from the previous commit.
This introduces new `*Pre` structures which are generated alongside the
existing `Guest` structures for example. The `*Pre` versions contain
`ComponentExportIndex` from the previous commit and serve as a path to
accelerating instantiation because all name lookups are skipped.
@alexcrichton alexcrichton requested a review from a team as a code owner June 12, 2024 20:05
@alexcrichton alexcrichton requested review from elliottt and removed request for a team June 12, 2024 20:05
@alexcrichton
Copy link
Member Author

ping @elliottt, mind taking a look at this? I can also reassign if you'd prefer too

Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

I read through the first three commits in detail, and skimmed the changes to updated files in the last two. This looks like a great change!

crates/wasmtime/src/compile.rs Outdated Show resolved Hide resolved

impl InstanceExportLookup for String {
fn lookup(&self, component: &Component) -> Option<ExportIndex> {
str::lookup(self, component)
Copy link
Member

@elliottt elliottt Jun 17, 2024

Choose a reason for hiding this comment

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

Does this need to be str::lookup(self.as_str(), component)? It's not obvious to me how the impl for str will be picked here, as the first argument will still be a &String.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah this ends up being a "cute" way to force rustc to do auto-deref without actually doing it yourself. The str::lookup function name here is the sugar-y form of <str as InstanceExportLookup>::lookup where we're explicitly selecting the impl on str so rustc knows ahead of time that the first argument must have type &str. Given the &String receiver it auto-derefs into &str.

(this only works as str::lookup isn't a method otherwise or on any other trait)

@alexcrichton alexcrichton added this pull request to the merge queue Jun 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 18, 2024
@alexcrichton alexcrichton added this pull request to the merge queue Jun 18, 2024
Merged via the queue into bytecodealliance:main with commit 3171ef6 Jun 18, 2024
36 checks passed
@alexcrichton alexcrichton deleted the export-lookup branch June 18, 2024 01:20
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jun 18, 2024
This commit is an implementation of component model semver compatibility
for export lookups. Previously in bytecodealliance#7994 component imports were made
semver-aware to ensure that bumping version numbers would not be a
breaking change. This commit implements the same feature for component
exports. This required some refactoring to move the definition of semver
compat around and the previous refactoring in bytecodealliance#8786 enables frontloading
this work to happen before instantiation.

Closes bytecodealliance#8395
github-merge-queue bot pushed a commit that referenced this pull request Jun 18, 2024
* Implement semver compatibility for exports

This commit is an implementation of component model semver compatibility
for export lookups. Previously in #7994 component imports were made
semver-aware to ensure that bumping version numbers would not be a
breaking change. This commit implements the same feature for component
exports. This required some refactoring to move the definition of semver
compat around and the previous refactoring in #8786 enables frontloading
this work to happen before instantiation.

Closes #8395

* Review comments

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants