Stream: git-wasmtime

Topic: wasmtime / PR #8786 Redesign how component exports work

Wasmtime GitHub notifications bot (Jun 12 2024 at 20:05):

alexcrichton opened PR #8786 from alexcrichton:export-lookup to bytecodealliance:main:

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 #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.

Wasmtime GitHub notifications bot (Jun 12 2024 at 20:05):

alexcrichton requested elliottt for a review on PR #8786.

Wasmtime GitHub notifications bot (Jun 12 2024 at 20:05):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8786.

Wasmtime GitHub notifications bot (Jun 17 2024 at 19:58):

alexcrichton commented on PR #8786:

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

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:10):

elliottt submitted PR review:

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!

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:10):

elliottt submitted PR review:

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!

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:10):

elliottt created PR review comment:

Does this need to be str::lookup(self.as_str(), component)? It's not obvious to me how the instance for str will be picked here.

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:10):

elliottt created PR review comment:

Would .cloned() work here?

            .cloned(),

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:11):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:12):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:50):

alexcrichton updated PR #8786.

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:52):

alexcrichton created PR review comment:

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)

Wasmtime GitHub notifications bot (Jun 17 2024 at 23:52):

alexcrichton has enabled auto merge for PR #8786.

Wasmtime GitHub notifications bot (Jun 18 2024 at 00:57):

alexcrichton updated PR #8786.

Wasmtime GitHub notifications bot (Jun 18 2024 at 00:57):

alexcrichton has enabled auto merge for PR #8786.

Wasmtime GitHub notifications bot (Jun 18 2024 at 01:20):

alexcrichton merged PR #8786.

Last updated: Nov 22 2024 at 17:03 UTC