Remove Sabre's manual insertion-order iteration and unnecessary sorts #9560
Conversation
The manual insertion-order tracking was initially added in 02a1939 (Qiskitgh-9012) during a complete rewrite of the scoring algorithms for Sabre to demonstrate that the new algorithm could be made fully RNG compatible. It maintained the identical iteration order to the previous iteration once the front-layer data structure was swapped from being a raw list to hash-based. The new data structure doesn't require the manual tracking to be reproducible as long as the iteration order is independent of the hash seed. This swaps the relevant places over the `IndexMap` to be able to remove a lot of the manual tracking. In casual testing, this didn't appear to have much effect on performance, but the resulting code is much simpler. The sorts (and deliberate canonicalisation of the swaps) was necessary to match RNG compatibility in the pre-relative-score Sabre, but since the swaps are now iterated through in a deterministic order and guaranteed to be generated only once each (the previous version used a hashset to remove duplicates), neither step is necessary. For a QV circuit of depth 5 at 1081 qubits on heavy hex, this is worth almost a 2x speedup in routing performance.
jakelishman
added
performance
Changelog: New Feature
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
|
Oops, I forgot to sort out the testing changes because of the new RNG patterns. Not sure what's going on with the gate-direction stuff, but I suspect it's related to a swap |
Pull Request Test Coverage Report for Build 4178888775
💛 - Coveralls |
Since you've made it so much more efficient, maybe it should be called |
|
Test non-determinism is fixed by #9568, but I'll need a new commit after that merges to update the chosen fixed layout in the currently flaky test to whatever that PR causes it to settle on. It occurring only on the Mac machines is a red herring; it's only related to |
There was a problem hiding this comment.
Overall this LGTM, the code is pretty straightforward (or this could just be because we discussed it at the time as part of the #9012 review) and the speed boost is a big win (also shaking loose bugs in other parts of the code was great). Just one small inline comment about expanding the release notes, but once that's there I think this should be good to go.
mtreinish
added
Changelog: API Change
Summary
The manual insertion-order tracking was initially added in 02a1939 (gh-9012) during a complete rewrite of the scoring algorithms for Sabre to demonstrate that the new algorithm could be made fully RNG compatible. It maintained the identical iteration order to the previous iteration once the front-layer data structure was swapped from being a raw list to hash-based.
The new data structure doesn't require the manual tracking to be reproducible as long as the iteration order is independent of the hash seed. This swaps the relevant places over the
IndexMapto be able to remove a lot of the manual tracking. In casual testing, this didn't appear to have much effect on performance, but the resulting code is much simpler.The sorts (and deliberate canonicalisation of the swaps) was necessary to match RNG compatibility in the pre-relative-score Sabre, but since the swaps are now iterated through in a deterministic order and guaranteed to be generated only once each (the previous version used a hashset to remove duplicates), neither step is necessary.
For a QV circuit of depth 5 at 1081 qubits on heavy hex, this is worth almost a 2x speedup in routing performance.
Details and comments
I've still got some fun ideas for improving the handling of the extended set by a more data-structure-explicit segregation into layers within it. I don't know at what point we should stop calling this "Sabre", because with all the changes we've made to the algorithm, the router only really bears a passing resemblance to the original paper.