Conversation
github-actions
bot
added
the
fuzzing
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Thanks! Feedback inline below.
There was a problem hiding this comment.
I think we will want to move our existing SCC implementation somewhere that it can be reused so that we don't have forks of the same code in the same tree.
I can do this and let you know when to rebase.
| // Kill self-edges to avoid cycles. | ||
| for (id, def) in self.type_defs.iter_mut() { | ||
| if def.supertype == Some(*id) { | ||
| def.supertype = None; | ||
| } | ||
| } |
There was a problem hiding this comment.
The DFS should handle this special case in a general way already, right? I think we can remove this.
| // Build group -> member list from current truth. | ||
| let mut members: BTreeMap<RecGroupId, Vec<TypeId>> = BTreeMap::new(); | ||
| for (id, def) in self.type_defs.iter() { | ||
| members.entry(def.rec_group).or_default().push(*id); | ||
| } |
There was a problem hiding this comment.
I don't think we should need to think about rec groups at all while breaking supertype cycles. The only constraint on a type's supertype is that the supertype is already defined at the point of the type's definition.
After breaking cycles, it should be sufficient to reorder types within a rec group as necessary, as a follow up fixup pass.
I bring this up because we don't want to, for example, artificially disallow a type subtyping another type defined earlier in its rec group. (We should have a test that defines a rec group of two types, where the first is the supertype of the second, and then calls fixup and make sure that the subtyping is preserved.)
| let mut path = Vec::new(); | ||
| let mut path_set = BTreeSet::new(); |
There was a problem hiding this comment.
Allocating these things within this double-nested loop is going to thrash the allocator and be quite slow. I think this DFS could be refactored to be more sympathatic to the machine and look something like this:
// A map from `TypeId` to the DFS index when we started traversing that type.
let mut dfs_pre_index = BTreeMap::new();
// A map from `TypeId` to the DFS index when we finished traversing that type.
let mut dfs_post_index = BTreeMap::new();
enum DfsEvent {
Enter,
Exit,
}
use DfsEvent::*;
let mut stack = self
.all_type_ids() // new method that returns `impl Iterator<Item = TypeId>`
.map(|ty| (Enter, ty))
.collect::<Vec<_>>();
let mut index = 0;
while let Some((event, ty)) = stack.pop() {
match event {
Enter => match dfs_pre_index.entry(ty) {
Entry::Occupied(_) => {},
Entry::Vacant(e) => {
e.insert(index);
index += 1;
stack.push((Exit, ty));
if let Some(sup_ty) = self.type_defs[ty].supertype {
if dfs_pre_index.contains_key(&sup_ty) && !dfs_post_index.contains_key(&sup_ty) {
// Cyclic subtyping! Break the cycle.
self.type_defs[ty].supertype = None;
} else {
stack.push((Event::Enter, sup_ty));
}
}
}
}
Exit => {
let old = dfs_post_index.insert(ty, index);
debug_assert!(old.is_none());
index += 1;
}
}
}| /// Merge rec-groups that participate in dependency cycles. | ||
| pub fn merge_rec_groups_via_scc(&mut self, rec_groups: &BTreeMap<RecGroupId, Vec<TypeId>>) { |
There was a problem hiding this comment.
It occurs to me that we don't actually even need SCCs yet, since subtyping doesn't care about rec group boundaries so long as the supertype is defined before the subtype. So I think we can delay all this stuff until we actually have struct fields (or array elements). This is nice because it means that support for subtyping will be able to land before we make the existing SCC algorithm reusable and all that.
| /// Topological sort of rec-groups. | ||
| pub fn sort_rec_groups_topo( |
There was a problem hiding this comment.
And we should probably make generic and reuse the DFS code from my last comment to do this topo sorting as well (although rec groups form a DAG, not a tree, but I think the code should work just fine for this case, perhaps with some minimal tweaks, as well).
| /// Topological sort of types by their supertype (supertype before subtype). | ||
| pub fn sort_types_by_supertype(&self) -> Vec<TypeId> { | ||
| #[derive(Copy, Clone, Debug)] | ||
| enum Event { | ||
| Enter, | ||
| Exit, | ||
| } | ||
|
|
||
| let mut stack: Vec<(Event, TypeId)> = self | ||
| .type_defs | ||
| .keys() | ||
| .copied() | ||
| .map(|id| (Event::Enter, id)) | ||
| .collect(); | ||
|
|
||
| stack.reverse(); | ||
|
|
||
| let mut sorted = Vec::with_capacity(self.type_defs.len()); | ||
| let mut seen = BTreeSet::<TypeId>::new(); | ||
|
|
||
| while let Some((event, id)) = stack.pop() { | ||
| match event { | ||
| Event::Enter => { | ||
| if seen.insert(id) { | ||
| stack.push((Event::Exit, id)); | ||
|
|
||
| if let Some(super_id) = self.type_defs[&id].supertype { | ||
| if !seen.contains(&super_id) { | ||
| stack.push((Event::Enter, super_id)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Event::Exit => { | ||
| sorted.push(id); | ||
| } | ||
| } | ||
| } | ||
| sorted | ||
| } |
There was a problem hiding this comment.
Ah okay, great. Yeah we should make this DFS code generic and reuse in our various places where we need to do a DFS.
| // Topological sort of rec-groups based on cross-group supertype edges. | ||
| let group_order: Vec<RecGroupId> = self.types.sort_rec_groups_topo(&rec_groups); |
There was a problem hiding this comment.
I already talked about how we can remove the SCC stuff in this PR, but for the future: this method should sort in place, rather than allocating and returning a new vector.
|
@khagankhan I just put a PR to make Wasmtime's existing SCC computation and DFS traversal stuff reusable, and I think this PR can be rebased on top of it: #12590 |
2 participants
Add support for struct subtyping
This PR adds support for one struct type being a subtype of another.
Changes
SubTypenow has:supertype: Option<TypeId>is_final: boolfinalwith 1/4 probabilityCycle Handling
Types::merge_rec_groups_via_scc(...):Types::break_type_cycles_in_rec_groups(...):supertype.Ordering
Enter/Exitenum) so supertypes come before subtypes.Fixups
Handled in
Types::fixup:finalsupertype its supertype becomesNoneNone)Mutator Update
duplicate_rec_groupnow actually duplicates.New tests added.
cc @fitzgen @eeide