Speedup constant factors in `LookaheadSwap` #8068

jakelishman · 2022-05-16T13:26:39Z

Summary

This picks some of the low-hanging fruit in LookaheadSwap, avoiding
recalculating various properties and entities that are already known,
and making some access patterns more efficient. It does not change the
complexity properties of the algorithm, which will still cause its
runtime to be excessive for large circuits.

Details and comments

This is worth about a 3x speedup in the pass itself, but it doesn't address any of the scaling issues that cause #5198, for example.

Using a circuit based on the one in #5198:

In [1]: from qiskit import QuantumCircuit
   ...: from qiskit.circuit.library import QuantumVolume
   ...: from qiskit.test.mock import FakeManhattan
   ...: from qiskit.converters import circuit_to_dag
   ...: from qiskit.transpiler import CouplingMap
   ...: from qiskit.transpiler.passes import LookaheadSwap
   ...:
   ...: dag = circuit_to_dag(
   ...:     QuantumCircuit(65).compose(QuantumVolume(4, seed=13).decompose(), [32, 33, 34, 35])
   ...: )
   ...: cm = CouplingMap(FakeManhattan().configuration().coupling_map)
   ...: swap = LookaheadSwap(cm, 5, 5)
   ...:
   ...: %timeit swap.run(dag)

gave 6.04(6)s before this commit, and 1.69(1)s after.

This pass isn't our preferred routing method any more, but I was looking at it since the randomised tests are spotting failures in it at the moment. This commit won't address the failures; the meaningful logic is unchanged, this commit just does more caching of intermediate values and uses more efficient paths to calculate things. I don't know the cause of the failures right now, but if I find it, I'll make a new PR.

It's possible we could consider dropping this routing pass in favour of one of the other, faster methods if its scaling problems are inherent.

This picks some of the low-hanging fruit in `LookaheadSwap`, avoiding recalculating various properties and entities that are already known, and making some access patterns more efficient. It does not change the complexity properties of the algorithm, which will still cause its runtime to be excessive for large circuits.

qiskit-bot · 2022-05-16T13:26:42Z

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:

@Qiskit/terra-core

coveralls · 2022-05-16T13:56:44Z

Pull Request Test Coverage Report for Build 2535942293

60 of 61 (98.36%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.0005%) to 84.282%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/routing/lookahead_swap.py	60	61	98.36%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/routing/lookahead_swap.py	2	93.98%

Totals
Change from base Build 2535468241:	0.0005%
Covered Lines:	54909
Relevant Lines:	65149

💛 - Coveralls

mtreinish

LGTM, one question inline about the _first_op_node() method before I tag as automerge.

I do think we should port this to rust after #8133 is fixed. This refactor makes it trivial to port as the structure is much closer to how it would be written in rust.

mtreinish · 2022-06-21T13:32:23Z

qiskit/transpiler/passes/routing/lookahead_swap.py

+def _first_op_node(dag):
+    """Get the first op node from a DAG."""
+    # This doesn't use `DAGCircuit.op_nodes` because that function always consumes the entire
+    # iterator to create a list, whereas we only need the first element.
+    return next(node for node in dag.nodes() if isinstance(node, DAGOpNode))


Reading through the code I don't think this matters but this is the first op node based on insertion order not necessarily topological order was that your intent or did you want the first node from a topological ordering of the dag?

This is just a direct port of the existing code - it previously did this exact thing but constructed the complete list just to take the first element. I was mostly trying to ensure the iteration order wasn't affect while just trying to speed it up.

Ok, that's what it looked like to me too but I wanted to confirm. The code just read a little weird to me because the "first" node could actually be something in the middle of the dag or at the end as it's dependent on insertion order.

mtreinish · 2022-06-21T13:34:34Z

qiskit/transpiler/passes/routing/lookahead_swap.py

+            continue
+        qubits = gate["partition"][0]
+        if len(qubits) == 2:
+            out += state.coupling_map.distance(layout_map[qubits[0]], layout_map[qubits[1]])


Not really something we should change here, but I typically avoid the distance() method and just access the inner distance matrix directly because besides the function call overhead you also have bounds checking which for something like this you know you're layout isn't going to return bounds outside the matrix. You could probably also use numpy's sum to speed this up (although when I've tried that in the past it wasn't really a noticeable difference).

Looking back, I think I was just doing a direct port of the code here - it previously used coupling_map.distance, just with an iterable unpacking of a generator expression. I unrolled the generator, but left the function call.

jakelishman

This pass is so slow, and (I think) pretty much completely superseded by SabreSwap, so I'm not 100% sure it's worth rewriting it in Rust. I think there's some extant bugs as well - it was originally randomised testing failures that caused me to look at the pass.

It's not entirely a coincidence the new form looks more Rust-like - I've always preferred pure functions that pass "state" objects around rather than making objects that are naturally pure API-implementation collections (transpiler passes) one-time-use by making them stateful of themselves.

jakelishman · 2022-06-21T13:42:45Z

qiskit/transpiler/passes/routing/lookahead_swap.py

+            continue
+        qubits = gate["partition"][0]
+        if len(qubits) == 2:
+            out += state.coupling_map.distance(layout_map[qubits[0]], layout_map[qubits[1]])


Looking back, I think I was just doing a direct port of the code here - it previously used coupling_map.distance, just with an iterable unpacking of a generator expression. I unrolled the generator, but left the function call.

jakelishman · 2022-06-21T13:44:32Z

qiskit/transpiler/passes/routing/lookahead_swap.py

+def _first_op_node(dag):
+    """Get the first op node from a DAG."""
+    # This doesn't use `DAGCircuit.op_nodes` because that function always consumes the entire
+    # iterator to create a list, whereas we only need the first element.
+    return next(node for node in dag.nodes() if isinstance(node, DAGOpNode))


This is just a direct port of the existing code - it previously did this exact thing but constructed the complete list just to take the first element. I was mostly trying to ensure the iteration order wasn't affect while just trying to speed it up.

jakelishman added performance Changelog: New Feature Include in the "Added" section of the changelog labels May 16, 2022

jakelishman added this to the 0.21 milestone May 16, 2022

jakelishman requested a review from a team as a code owner May 16, 2022 13:26

jakelishman added 2 commits May 16, 2022 18:15

Put comment in correct location

34d9894

Merge remote-tracking branch 'ibm/main' into speedup-lookahead-swap

1920c4d

mtreinish self-assigned this Jun 14, 2022

mtreinish approved these changes Jun 21, 2022

View reviewed changes

jakelishman commented Jun 21, 2022

View reviewed changes

mtreinish added the automerge label Jun 21, 2022

Merge branch 'main' into speedup-lookahead-swap

3270c8f

mergify bot merged commit 0e3e68d into Qiskit:main Jun 21, 2022

jakelishman deleted the speedup-lookahead-swap branch June 22, 2022 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speedup constant factors in `LookaheadSwap` #8068

Speedup constant factors in `LookaheadSwap` #8068

jakelishman commented May 16, 2022

qiskit-bot commented May 16, 2022

coveralls commented May 16, 2022 •

edited

Loading

mtreinish left a comment

mtreinish Jun 21, 2022

jakelishman Jun 21, 2022

mtreinish Jun 21, 2022

mtreinish Jun 21, 2022

jakelishman Jun 21, 2022

jakelishman left a comment

jakelishman Jun 21, 2022

jakelishman Jun 21, 2022

Speedup constant factors in LookaheadSwap #8068

Speedup constant factors in LookaheadSwap #8068

Conversation

jakelishman commented May 16, 2022

Summary

Details and comments

qiskit-bot commented May 16, 2022

coveralls commented May 16, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2535942293

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Jun 21, 2022

Choose a reason for hiding this comment

jakelishman Jun 21, 2022

Choose a reason for hiding this comment

mtreinish Jun 21, 2022

Choose a reason for hiding this comment

mtreinish Jun 21, 2022

Choose a reason for hiding this comment

jakelishman Jun 21, 2022

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Jun 21, 2022

Choose a reason for hiding this comment

jakelishman Jun 21, 2022

Choose a reason for hiding this comment

Speedup constant factors in `LookaheadSwap` #8068

Speedup constant factors in `LookaheadSwap` #8068

coveralls commented May 16, 2022 •

edited

Loading