fix: Replace non-deterministic iterations on hash maps#3055
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3055 +/- ##
==========================================
+ Coverage 81.09% 81.20% +0.11%
==========================================
Files 241 242 +1
Lines 45054 45629 +575
Branches 38822 39385 +563
==========================================
+ Hits 36536 37055 +519
- Misses 6542 6586 +44
- Partials 1976 1988 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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)
| /// 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!
| .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?
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>
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.