fix: Replace non-deterministic iterations on hash maps#3055
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3055 +/- ##
==========================================
+ Coverage 81.09% 81.29% +0.19%
==========================================
Files 241 242 +1
Lines 45054 46823 +1769
Branches 38822 40506 +1684
==========================================
+ Hits 36536 38064 +1528
- Misses 6542 6768 +226
- Partials 1976 1991 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| blocks: Vec<Node>, | ||
| ) -> (Node, Node, Node) { | ||
| let mut other_blocks = h.children(cfg).collect::<HashSet<_>>(); | ||
| let mut other_blocks = h.children(cfg).collect::<BTreeSet<_>>(); |
There was a problem hiding this comment.
Not necessary - we only iterate through this to do an assert (line 489, five lines down) - you could either expect on the loop or rewrite to an .all (if that helps?)
| return Err(NodeLinkingError::NotChildOfRoot(sn)); | ||
| } | ||
| for sn in other.children(other.module_root()) { | ||
| let Some(dirv) = children.get(&sn) else { |
There was a problem hiding this comment.
Eeek! This means we're now going through other.children which might be much bigger than children.
Nondeterminism only affects the error reported (NodeMultiplyReplaced), but to prevent that, you are probably better off copying children into a BTreeSet (avoiding the need for min_directive_key and so on)
There was a problem hiding this comment.
Rolled back, and added some expects.
There was a problem hiding this comment.
Have you? You're still going through other.children, I mean I guess it's not terrible (you'll copy them all when linking happens)...you decided not to just put children into a BTreeThing
| /// sibling graph (since these must be the input and output nodes). | ||
| fn order_siblings_by_node_index(&mut self) { | ||
| let mut node_children: HashMap<Node, Vec<Node>> = HashMap::default(); | ||
| let mut node_children = Vec::new(); |
There was a problem hiding this comment.
I'm not convinced there's any actual nondeterminism here; processing each element of node_children seems independent.
However, node_children seems like an enormous structure. Why not just merge the loops (move the self.hierarchy stuff in the second loop, into the first)? You'd need to do for node in self.nodes().collect::<Vec<_>>() but that vec would be much smaller than node_children after all!
There was a problem hiding this comment.
Done! We're only mutating the hierarchy, so the borrow checker is happy if we iterate over self.graph's nodes instead.
| .map(|&PatchNode(id, node)| (id, node)) | ||
| .into_grouping_map() | ||
| .collect::<BTreeSet<_>>(); | ||
| let mut new_invalid_nodes = IndexMap::<_, BTreeSet<_>>::new(); |
There was a problem hiding this comment.
This feels independent/driveby? Map on LHS is BTreeMap not HashMap?
There was a problem hiding this comment.
The groups themselves are stored using hashes.
.into_grouping_map().collect<T>() produces a HashMap<T>.
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Love it, thanks @aborgna-q - a few suggestions but happy to approve unless you'd rather @mark-koch had a final look over?
Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>
|
|
||
| // 6. Transfer to keys of `transfers` children of the corresponding values. | ||
| #[expect( | ||
| clippy::iter_over_hash_type, |
There was a problem hiding this comment.
Of course, if the same value (old_parent) appeared twice in the map, we'd get completely different results - they'd get moved to the first new_parent (and then there'd to nothing to move to whichever new_parent came second)....
There was a problem hiding this comment.
Ouch, good catch.
Repeated values should cause a hard error.
I added the check (previously it was only a debug_assert), and documented it in Replacement::adoptions.
| } | ||
|
|
||
| // 7. Remove remaining nodes | ||
| #[expect( |
There was a problem hiding this comment.
Agreed this is harmless. But we could remove the expect by changing private fn get_removed_nodes to return a BTreeSet....
| roots.insert(other.entrypoint(), parent); | ||
| other.set_parent(other.entrypoint(), other.module_root()); | ||
| }; | ||
| #[expect( |
There was a problem hiding this comment.
You could move the roots.insert out of the loop using extend, which avoids the lint (whether because it really knows that the keys must all be unique, or it just doesn't understand extend, I'm not sure..). But yeah, removing the lint requires collecting all the children-whose-grandchildren-to-remove into a list, which again, makes me doubt that the linter is actually all that clever...
2 participants
Enables the
iter_over_hash_typeclippy lint, and fixes all cases of iteration over hashmaps and hashsets that could cause non-deterministic outputs.