Support multiple supertypes in everything but validation - #2652
alexcrichton wants to merge 2 commits into
Conversation
Upstream a new spec test has been added that a type with multiple supetypes is considered invalid. This is distinct from malformed meaning that the binary doesn't even parse, but by being invalid that means that the binary parses but is dynamically considered invalid. This test requires handling multiple supertypes in all locations throughout the tooling here, for example `wast`, `wasm-encoder`, and `wasmparser`. Notably `wasmparser` now has a `Vec<u32>` for supertype indices, and lengths > 1 are rejected during validation. While here this is changing all of the `dump` test outputs anyway so this switches to printing types in a more human-readable format.
| /// The list of supertype indexes. As of GC MVP, there can be at most one | ||
| /// supertype. | ||
| pub supertype_idx: Option<u32>, | ||
| pub supertype_idxs: Vec<u32>, |
There was a problem hiding this comment.
SmallVec to avoid the heap allocation in practice for valid modules?
There was a problem hiding this comment.
How strongly do you feel about that? This has come up a few times in the past about using smallvec in wasmparser and other crates here, so this isn't new, but so far we haven't added the dependency. This would add a new dep to all of these crates, and the only real perf-critical one is wasmparser which is already, before this PR, collecting the results into the heap and then taking it back out.
If smallvec or similar were in libstd I'd use it without hesitation, but as an extra dep it's something extra for all users to pull in and depend on, and wasmparser in particular shows up in a good number of dependency trees. Keeping our deps slim for minimal feature builds (e.g. without the component-model) feels relatively important at least.
There was a problem hiding this comment.
It feels like a very light dep to me, but if we're concerned about that, then I think we can do some small shenanigans to make it optional while making the uses inside the crates not too bad. Luckily that is pretty simple and concise, so not too annoying to duplicate across each crate.
There was a problem hiding this comment.
That unfortunately wouldn't work though b/c toggling a Cargo feature would be able to cause compile-time breakages -- basically it's public knowledge that SmallVec<...> == Vec<T> so when the crate dep is pulled in it'd break builds that relied on such equivalence. We've historically solved this in wasmparser with collection wrappers but that requires duplicating the API surface area of collections which I think crosses the threshold of "probably shouldn't copy across crates".
I suppose another way to say what I'm thinking is that I don't think that the crate dependency is worth it unless we've got benchmarks showing such. At this time I don't think we do have such benchmarks, and at least for Wasmtime's use cases I don't think validation/parsing really factors into any of them, so I don't think we actually have a benchmark-driven use case for taking a new dependency at this time
| pub is_final: bool, | ||
| /// The list of supertype indexes. As of GC MVP, there can be at most one supertype. | ||
| pub supertype_idx: Option<PackedIndex>, | ||
| pub supertype_idxs: Vec<PackedIndex>, |
| pub shared: bool, | ||
| /// The declared parent type of this definition. | ||
| pub parent: Option<Index<'a>>, | ||
| pub parents: Vec<Index<'a>>, |
2 participants
Upstream a new spec test has been added that a type with multiple supetypes is considered invalid. This is distinct from malformed meaning that the binary doesn't even parse, but by being invalid that means that the binary parses but is dynamically considered invalid. This test requires handling multiple supertypes in all locations throughout the tooling here, for example
wast,wasm-encoder, andwasmparser. Notablywasmparsernow has aVec<u32>for supertype indices, and lengths > 1 are rejected during validation.While here this is changing all of the
dumptest outputs anyway so this switches to printing types in a more human-readable format.