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

Overhaul and refactor some internals of wit-component #1810

Merged

Conversation

alexcrichton
Copy link
Member

This commit is a lead-up to the changes proposed in #1774. The
wit-component crate is quite old and has gone through many iterations
of the component model and it's very much showing its age in a few
places. Namely the correlation between component model names and core
wasm names is open-coded in many places throughout validation and
encoding of a component. This makes the changes in #1774 where the names
may be different (e.g. core wasm 0.2.0 might import component 0.2.1).

Making this change was inevitably going to require quite a lot of
refactoring of wit-component, so that's what this commit does. It's a
pretty large rewrite of the internals of validation of a core wasm
module and adapter prior to creating a component. The metadata produced
by this pass is now represented in a completely different format. The
metadata is extensively used throughout the encoding process so encoding
has been heavily refactored as well.

The overall idea is that the previous "grab bag" of various items here
and there produced from validation are now all unified into a single
ImportMap and ExportMap for a module. These maps track all the
various kinds of imports and exports and how they map back to core wasm
names. Notably this means that the logic to correlate core wasm names
with component model names is now happening in just one location (in
theory) as opposed to implicitly all throughout encoding. I've
additionally taken this opportunity to subjectively simplify much of the
encoding process around managing instantiations of core wasm modules and
adapters.

One of the main changes in this commit is that it does away with code
structure such as "do the thing for WIT" then "do the thing for
resources" then "do the thing for other resources" and finally "do the
thing for adapters". This was difficult to understand every time I came
back to it and I can't imagine was easy for anyone else to understand
either. All imports are now handled in a single location and it's
intended to be much better separated who's responsible for what. For
example the code satisfying an import is decoupled from what the import
is going to be named and how it's provided to the main core wasm module.

Overall the intention is that this does not either enhance the
functionality of wit-component nor regress it. Lots of tests have
changed but I've tried to verify it's just things moving around as
opposed to anything that has a different semantic meaning. A future PR
for #1774 will enhance the logic of connecting core wasm imports to WIT
imports but that's deferred for a future PR.

This commit is a lead-up to the changes proposed in bytecodealliance#1774. The
`wit-component` crate is quite old and has gone through many iterations
of the component model and it's very much showing its age in a few
places. Namely the correlation between component model names and core
wasm names is open-coded in many places throughout validation and
encoding of a component. This makes the changes in bytecodealliance#1774 where the names
may be different (e.g. core wasm 0.2.0 might import component 0.2.1).

Making this change was inevitably going to require quite a lot of
refactoring of `wit-component`, so that's what this commit does. It's a
pretty large rewrite of the internals of validation of a core wasm
module and adapter prior to creating a component. The metadata produced
by this pass is now represented in a completely different format. The
metadata is extensively used throughout the encoding process so encoding
has been heavily refactored as well.

The overall idea is that the previous "grab bag" of various items here
and there produced from validation are now all unified into a single
`ImportMap` and `ExportMap` for a module. These maps track all the
various kinds of imports and exports and how they map back to core wasm
names. Notably this means that the logic to correlate core wasm names
with component model names is now happening in just one location (in
theory) as opposed to implicitly all throughout encoding. I've
additionally taken this opportunity to subjectively simplify much of the
encoding process around managing instantiations of core wasm modules and
adapters.

One of the main changes in this commit is that it does away with code
structure such as "do the thing for WIT" then "do the thing for
resources" then "do the thing for other resources" and finally "do the
thing for adapters". This was difficult to understand every time I came
back to it and I can't imagine was easy for anyone else to understand
either. All imports are now handled in a single location and it's
intended to be much better separated who's responsible for what. For
example the code satisfying an import is decoupled from what the import
is going to be named and how it's provided to the main core wasm module.

Overall the intention is that this does not either enhance the
functionality of wit-component nor regress it. Lots of tests have
changed but I've tried to verify it's just things moving around as
opposed to anything that has a different semantic meaning. A future PR
for bytecodealliance#1774 will enhance the logic of connecting core wasm imports to WIT
imports but that's deferred for a future PR.
@alexcrichton
Copy link
Member Author

@dicej one thing I'm particularly keen to get your eyes on is the second commit here. I think that the new feature of "cache core aliases if they were already created" is what's causing most of the changes there, but I wanted to double check. All test updates in the first commit I tried to go over by hand myself, but the second commit all had a similar shape of changes and were related to the linking functionality so I'm less familiar with them. Just want to make sure nothing surprising happened there.

Copy link
Collaborator

@dicej dicej left a comment

Choose a reason for hiding this comment

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

LGTM, and I ran the componentize-py test suite (which relies heavily on shared-everything linking) using this branch -- all green.

@alexcrichton alexcrichton added this pull request to the merge queue Sep 23, 2024
Merged via the queue into bytecodealliance:main with commit 6fc5d68 Sep 23, 2024
30 checks passed
@alexcrichton alexcrichton deleted the refactor-wit-component branch September 23, 2024 15:52
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.

2 participants