Type/Expression Tandem

[kentslaney]

Connections
Addresses #6788

Description
With the introduction of override-sized arrays, expressions can now refer to types. The existing compactor was built assuming a one directional dependency, meaning that unused types wouldn't have the supporting expressions removed. This PR removes that directionality assumption by tracing the types used by expressions in order to propagate usage. It also validates that any expression A that relies on a type that relies on another expression B has index A > B.

Testing
This is tested by checking that going from expression to type and back, along with the opposite, is correctly marked as used. The updated validation is checked by ensuring an error is raised when an out of order dependency is introduced.

Checklist

• Run cargo fmt.
• Run taplo format.
• Run cargo clippy. If applicable, add:
  • --target wasm32-unknown-unknown
  • --target wasm32-unknown-emscripten
• Run cargo xtask test to run tests.
• Add change to CHANGELOG.md. See simple instructions inside file.

[jimblandy]

@kentslaney Why is Validator::well_ordered_deps necessary? Doesn't the existing code already check all these handles?

[kentslaney]

What happened with the validation was that I needed something different than cycle-free, but, because of how cycle-free was validated, it actually provided the guarantee that I needed.

[jimblandy]

I left some detailed comments but they may be moot, because I would like to ask for some deeper changes that will probably make the code I commented on go away anyway.

Specifically, ExpressionTracer::trace_type ends up using TypeTracer::trace_type to do a recursive traversal of the type arena. But because the type arena is a DAG, it should always be sufficient to make a single pass from end to front; recursion should never be needed. Also, because this recursion doesn't quit early when it sees a type it's already visited, it could take exponential time in the case of types like this:

struct A { }
struct B { m: A, n: A }
struct C { m: B, n: B }
struct D { m: C, n: C }
...

I mean, the struct's size is also exponential in the nesting depth, so there are going to be some limits imposed on this kind of thing anyway, but it's not great for compaction to get caught up in that too.

[jimblandy]

What happened with the validation was that I needed something different than cycle-free, but, because of how cycle-free was validated, it actually provided the guarantee that I needed.

Yes, this is a fair point - we validate not only that the arenas are cycle-free, but that handles only refer to earlier handles, which is a stronger condition.

[jimblandy]

@kentslaney Hmm. I'd been assuming that there must be some analogue of the tactic we use in Validator::validate_module_handles that we could use in the compactor, that handles everything in a single pass. But as I'm thinking about it, I'm not sure there is a way after all. It may be the case that we need the deep traversal into types. But the way it may visit the same type many times gets worrisome.

I was thinking, oh, if the type is already marked as used, then you can stop the recursion there. But types could be marked as used even if we haven't looked into which expressions they use, so the flag indicating that the type is used doesn't imply that we don't need to traverse it.

[kentslaney]

Oh, I actually did mean to stop the recursion if the type is already marked as used. Shouldn't step 1 ensure that any types marked as used have the corresponding expressions marked as used?

[kentslaney]

For some reason I had it in my head that because the type arena was merging things that meant you could have out of order dependencies which made things more complicated. I think you're right that this could be simpler since the current version assumes types are an unordered set.

[jimblandy]

For some reason I had it in my head that because the type arena was merging things that meant you could have out of order dependencies which made things more complicated. I think you're right that this could be simpler since the current version assumes types are an unordered set.

Yeah, that should never happen, because we don't mutate types. They can only ever refer to stuff that was already in the arenas when they were created.

I think if you made a pass over the type arena from front to back and accumulated a map of the highest-indexed expression referred to by the types up to a given handle, then you could traverse types and expressions from back to front, using that map to decide how many expressions to process, and you'd always know that when you reached an expression marked unused, it was really, really never used, no matter what else you find in the types arena.

That sounds a lot more delicate than just recursing into types, if we could trust the types "used" bits to tell us when to stop traversing.

[kentslaney]

I think if you made a pass over the type arena from front to back and accumulated a map of the highest-indexed expression referred to by the types up to a given handle, then you could traverse types and expressions from back to front, using that map to decide how many expressions to process, and you'd always know that when you reached an expression marked unused, it was really, really never used, no matter what else you find in the types arena.

That sounds a lot more delicate than just recursing into types, if we could trust the types "used" bits to tell us when to stop traversing.

I implemented this in #6934. I actually think your solution is a simpler implementation, but it's up to you. (The diff is easiest to view without whitespace changes.)