qiskit-bot
commented
3 weeks ago One or more of the following people are relevant to this code:
@Qiskit/terra-core@kevinhartman@mtreinish
Closed mtreinish closed 2 weeks ago
qiskit-bot
commented
3 weeks ago One or more of the following people are relevant to this code:
@Qiskit/terra-core@kevinhartman@mtreinish
mtreinish
commented
3 weeks ago I ran a quick asv benchmark on this and it's yielding about a ~10x speed up for larger circuits:
Benchmarks that have improved:
| Change | Before [86a1b496] <dag_to_circuit~1> | After [c6da91cd] | Ratio | Benchmark (Parameter) |
|----------|----------------------------------------|---------------------|---------|--------------------------------------------------------------|
| - | 30.0±0.2μs | 17.2±0.2μs | 0.58 | converters.ConverterBenchmarks.time_dag_to_circuit(1, 8) |
| - | 43.2±0.1μs | 21.4±0.07μs | 0.49 | converters.ConverterBenchmarks.time_dag_to_circuit(2, 8) |
| - | 76.7±0.8μs | 34.1±0.7μs | 0.44 | converters.ConverterBenchmarks.time_dag_to_circuit(5, 8) |
| - | 173±0.5μs | 71.8±3μs | 0.42 | converters.ConverterBenchmarks.time_dag_to_circuit(14, 8) |
| - | 111±0.7μs | 46.7±1μs | 0.42 | converters.ConverterBenchmarks.time_dag_to_circuit(8, 8) |
| - | 371±2μs | 147±2μs | 0.4 | converters.ConverterBenchmarks.time_dag_to_circuit(32, 8) |
| - | 599±2μs | 237±9μs | 0.4 | converters.ConverterBenchmarks.time_dag_to_circuit(53, 8) |
| - | 239±1μs | 94.4±1μs | 0.39 | converters.ConverterBenchmarks.time_dag_to_circuit(20, 8) |
| - | 5.10±0.04ms | 842±20μs | 0.17 | converters.ConverterBenchmarks.time_dag_to_circuit(53, 128) |
| - | 164±2μs | 26.4±0.3μs | 0.16 | converters.ConverterBenchmarks.time_dag_to_circuit(1, 128) |
| - | 3.00±0.01ms | 478±20μs | 0.16 | converters.ConverterBenchmarks.time_dag_to_circuit(32, 128) |
| - | 246±0.7μs | 34.7±0.3μs | 0.14 | converters.ConverterBenchmarks.time_dag_to_circuit(2, 128) |
| - | 1.94±0.02ms | 273±7μs | 0.14 | converters.ConverterBenchmarks.time_dag_to_circuit(20, 128) |
| - | 1.33±0.01ms | 179±2μs | 0.13 | converters.ConverterBenchmarks.time_dag_to_circuit(14, 128) |
| - | 513±5μs | 65.8±3μs | 0.13 | converters.ConverterBenchmarks.time_dag_to_circuit(5, 128) |
| - | 785±4μs | 103±1μs | 0.13 | converters.ConverterBenchmarks.time_dag_to_circuit(8, 128) |
| - | 20.3±0.09ms | 1.97±0.02ms | 0.1 | converters.ConverterBenchmarks.time_dag_to_circuit(14, 2048) |
| - | 3.56±0.02ms | 304±7μs | 0.09 | converters.ConverterBenchmarks.time_dag_to_circuit(2, 2048) |
| - | 7.34±0.05ms | 672±9μs | 0.09 | converters.ConverterBenchmarks.time_dag_to_circuit(5, 2048) |
| - | 30.9±0.6ms | 2.70±0.04ms | 0.09 | converters.ConverterBenchmarks.time_dag_to_circuit(5, 8192) |
| - | 11.8±0.1ms | 1.06±0.01ms | 0.09 | converters.ConverterBenchmarks.time_dag_to_circuit(8, 2048) |
| - | 50.9±0.4ms | 4.60±0.1ms | 0.09 | converters.ConverterBenchmarks.time_dag_to_circuit(8, 8192) |
| - | 2.19±0.01ms | 175±3μs | 0.08 | converters.ConverterBenchmarks.time_dag_to_circuit(1, 2048) |
| - | 8.92±0.07ms | 709±30μs | 0.08 | converters.ConverterBenchmarks.time_dag_to_circuit(1, 8192) |
| - | 14.6±0.09ms | 1.18±0.01ms | 0.08 | converters.ConverterBenchmarks.time_dag_to_circuit(2, 8192) |
Benchmarks that have stayed the same:
| Change | Before [86a1b496] <dag_to_circuit~1> | After [c6da91cd] | Ratio | Benchmark (Parameter) |
|----------|----------------------------------------|---------------------|---------|--------------------------------------------------------------|
| | n/a | n/a | n/a | converters.ConverterBenchmarks.time_dag_to_circuit(14, 8192) |
| | n/a | n/a | n/a | converters.ConverterBenchmarks.time_dag_to_circuit(20, 2048) |
| | n/a | n/a | n/a | converters.ConverterBenchmarks.time_dag_to_circuit(20, 8192) |
| | n/a | n/a | n/a | converters.ConverterBenchmarks.time_dag_to_circuit(32, 2048) |
| | n/a | n/a | n/a | converters.ConverterBenchmarks.time_dag_to_circuit(32, 8192) |
| | n/a | n/a | n/a | converters.ConverterBenchmarks.time_dag_to_circuit(53, 2048) |
| | n/a | n/a | n/a | converters.ConverterBenchmarks.time_dag_to_circuit(53, 8192) |
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.
coveralls
commented
3 weeks ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| crates/circuit/src/converters.rs | 37 | 40 | 92.5% | ||
| <!-- | Total: | 69 | 72 | 95.83% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| crates/circuit/src/dag_node.rs | 1 | 77.11% | ||
| crates/qasm2/src/expr.rs | 1 | 94.02% | ||
| crates/accelerate/src/two_qubit_decompose.rs | 1 | 90.82% | ||
| qiskit/transpiler/passes/synthesis/unitary_synthesis.py | 2 | 88.26% | ||
| crates/qasm2/src/lex.rs | 3 | 92.48% | ||
| qiskit/synthesis/two_qubit/xx_decompose/decomposer.py | 7 | 90.91% | ||
| crates/qasm2/src/parse.rs | 12 | 97.15% | ||
| <!-- | Total: | 27 | --> |
| Totals | |
|---|---|
| Change from base Build 10834414649: | 0.2% |
| Covered Lines: | 55413 |
| Relevant Lines: | 62218 |
Summary
This commit migrates the dag_to_circuit() converter function implementation to rust. The core of this is having a DAGCircuit to CircuitData converter function in rust. To do this efficiently we just copy the BitDatas and Interners to the new circuit data object being created and then just iterate over all the packed instructions in the circuit. To facilitate this a new constructor for CircuitData is added that takes an iterator of PackedInstructions and the BitDatas and Interners and builds a new circuit data from that.
Details and comments