Fix open world public types#8792
Conversation
In an open world, we have to assume that the environment may cast any public type down to any of its subtypes and expect that to work before and after optimization. We therefore must make public visibility propagate to subtypes. But we can be more precise than this, because some types are public not because they can cross the module boundary, but rather because they are reachable in the definition of some other type that can cross the module boundary. Subtypes of such public types do not need to be public, since their values will never cross the module boundary. In addition, we do not need to make public the subtypes of types that are exposed only as exact references. Allow Unsubtyping to operate in open-world mode and use it in tests that the visibility propagation works correctly. Fixes #8718.
tlively
requested review from
kripken and
stevenfontanella
and removed request for
a team
| // Insert or upgrade a type's exposure in the `visited` map. If a type's | ||
| // exposure is upgraded, we re-push it to the worklist to update the | ||
| // propagation to related types. | ||
| auto visit = [&](HeapType type, Exposure state) { |
There was a problem hiding this comment.
Perhaps rename visit to update? "visit" is something we use in wasm-traversal heavily throughout the codebase.
There was a problem hiding this comment.
I renamed it to "markPublic"
| // Rec group members must also be public, but do not necessarily cross the | ||
| // module boundary. | ||
| for (auto member : curr.getRecGroup()) { | ||
| visit(member, Exposure::NotExposed); |
There was a problem hiding this comment.
What does it actually do, to update with "not exposed"? Isn't not being exposed the default?
There was a problem hiding this comment.
The three exposure levels here affect how propagation works, but in the end they all correspond to public types. A "not exposed" type here is public only because its identity must be stable to keep the identity of some other public type stable.
There was a problem hiding this comment.
I've updated the comments on the enum definition to hopefully make this clearer.
| auto typeInfo = collectHeapTypeInfo(wasm, | ||
| worldMode, | ||
| TypeInclusion::AllTypes, | ||
| VisibilityHandling::FindVisibility); |
There was a problem hiding this comment.
This may make us slower - unavoidable, but maybe worth measuring to see how bad this is, and if we want to look for mitigations?
There was a problem hiding this comment.
At least it doesn't make closed-world mode slower. And I expect that caching type visibility on the module the same way we cache type names, indices, and effects would be a massive speedup in both closed-world and open-world modes because it would let us mostly avoid traversing the module just to find types.
I can try to measure the performance, but I don't think landing this should wait on that.
There was a problem hiding this comment.
Another idea I had in mind here was this: if we measure that this makes open world much slower while also making it much less effective (as func or externref is usually exported, etc.), then perhaps we would disable some passes by default in open world for now?
As we made things faster, we could re-enable them.
But if the slowdown is not large, that's not a concern. I'm just thinking about the fact that open world is the default, so many users could be affected by any slowdown.
There was a problem hiding this comment.
then perhaps we would disable some passes by default in open world for now?
This is de facto close to what we have today since so many of the type-rewriting passes throw errors when run in open-world mode.
But we'll have to measure the impact to decide what to do.
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $caller (export "out") (param $any anyref) | ||
| (@binaryen.js.called) |
There was a problem hiding this comment.
Why is this changed from an export?
There was a problem hiding this comment.
The export making any public was inhibiting all optimization. Using @binaryen.js.called also achieves the goal of telling GUFA this function can be called, but does not make any types public. The new test below shows what happens when we still use an export.
| (export "t" (tag $t)) | ||
| ) | ||
|
|
||
| ;; Function signature exact vs inexact params and results. |
There was a problem hiding this comment.
Please explain what to expect optimized in this test.
2 participants
In an open world, we have to assume that the environment may cast any public type down to any of its subtypes and expect that to work before and after optimization. We therefore must make public visibility propagate to subtypes.
But we can be more precise than this, because some types are public not because they can cross the module boundary, but rather because they are reachable in the definition of some other type that can cross the module boundary. Subtypes of such public types do not need to be public, since their values will never cross the module boundary. In addition, we do not need to make public the subtypes of types that are exposed only as exact references.
Allow Unsubtyping to operate in open-world mode and use it in tests that the visibility propagation works correctly.
Fixes #8718.