From e8e5efc50203e29c0c053a12f906aeb758572761 Mon Sep 17 00:00:00 2001 From: Matthew Treinish Date: Mon, 28 Oct 2024 16:15:10 -0400 Subject: [PATCH 1/2] Fix performance regression in UnitarySynthesis This commit fixes a performance regression that was introduced in PR #13141. When the pass is looking up the preferred synthesis direction for a unitary based on the connectvity constraints the connectivity was being provided as a PyList. To look up the edge in connectivity set this meant we needed to iterate over the list and then create a set that rust could lookup if it contains an edge or it's reverse. This has significant overhead because its iterating via python and also iterating per decomposition. This commit addresses this by changing the input type to be a HashSet from Python so Pyo3 will convert a pyset directly to a HashSet once at call time and that's used by reference for lookups directly instead of needing to iterate over the list each time. --- crates/accelerate/src/unitary_synthesis.rs | 22 +++++++------------ .../passes/synthesis/unitary_synthesis.py | 2 +- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/crates/accelerate/src/unitary_synthesis.rs b/crates/accelerate/src/unitary_synthesis.rs index 1931f1e97b2b..b5ef66db4f1e 100644 --- a/crates/accelerate/src/unitary_synthesis.rs +++ b/crates/accelerate/src/unitary_synthesis.rs @@ -27,7 +27,7 @@ use smallvec::{smallvec, SmallVec}; use pyo3::intern; use pyo3::prelude::*; -use pyo3::types::{IntoPyDict, PyDict, PyList, PyString}; +use pyo3::types::{IntoPyDict, PyDict, PyString}; use pyo3::wrap_pyfunction; use pyo3::Python; @@ -225,7 +225,7 @@ fn py_run_main_loop( qubit_indices: Vec, min_qubits: usize, target: &Target, - coupling_edges: &Bound<'_, PyList>, + coupling_edges: HashSet<[PhysicalQubit; 2]>, approximation_degree: Option, natural_direction: Option, ) -> PyResult { @@ -268,7 +268,7 @@ fn py_run_main_loop( new_ids, min_qubits, target, - coupling_edges, + coupling_edges.clone(), approximation_degree, natural_direction, )?; @@ -352,7 +352,7 @@ fn py_run_main_loop( py, unitary, ref_qubits, - coupling_edges, + &coupling_edges, target, approximation_degree, natural_direction, @@ -383,7 +383,7 @@ fn run_2q_unitary_synthesis( py: Python, unitary: Array2, ref_qubits: &[PhysicalQubit; 2], - coupling_edges: &Bound<'_, PyList>, + coupling_edges: &HashSet<[PhysicalQubit; 2]>, target: &Target, approximation_degree: Option, natural_direction: Option, @@ -794,7 +794,7 @@ fn preferred_direction( decomposer: &DecomposerElement, ref_qubits: &[PhysicalQubit; 2], natural_direction: Option, - coupling_edges: &Bound<'_, PyList>, + coupling_edges: &HashSet<[PhysicalQubit; 2]>, target: &Target, ) -> PyResult> { // Returns: @@ -830,14 +830,8 @@ fn preferred_direction( Some(false) => None, _ => { // None or Some(true) - let mut edge_set = HashSet::new(); - for item in coupling_edges.iter() { - if let Ok(tuple) = item.extract::<(usize, usize)>() { - edge_set.insert(tuple); - } - } - let zero_one = edge_set.contains(&(qubits[0].0 as usize, qubits[1].0 as usize)); - let one_zero = edge_set.contains(&(qubits[1].0 as usize, qubits[0].0 as usize)); + let zero_one = coupling_edges.contains(&qubits); + let one_zero = coupling_edges.contains(&[qubits[1], qubits[0]]); match (zero_one, one_zero) { (true, false) => Some(true), diff --git a/qiskit/transpiler/passes/synthesis/unitary_synthesis.py b/qiskit/transpiler/passes/synthesis/unitary_synthesis.py index 883dbb6c4d68..cf9acdc890c2 100644 --- a/qiskit/transpiler/passes/synthesis/unitary_synthesis.py +++ b/qiskit/transpiler/passes/synthesis/unitary_synthesis.py @@ -505,7 +505,7 @@ def run(self, dag: DAGCircuit) -> DAGCircuit: if self.method == "default" and isinstance(kwargs["target"], Target): _coupling_edges = ( - list(self._coupling_map.get_edges()) if self._coupling_map is not None else [] + set(self._coupling_map.get_edges()) if self._coupling_map is not None else set() ) out = run_default_main_loop( From eae8ba67876922f1059e256fe27ec177dcad1f50 Mon Sep 17 00:00:00 2001 From: Matthew Treinish Date: Mon, 28 Oct 2024 17:07:51 -0400 Subject: [PATCH 2/2] Avoid constructing plugin data views with default plugin If we're using the default plugin the execution will happen in rust and we don't need to build the plugin data views that are defined in the plugin interface. Profiling the benchpress test using the hamlib hamiltonian: ham_graph-2D-grid-nonpbc-qubitnodes_Lx-5_Ly-186_h-0.5-all-to-all that caught this originally regression was showing an inordinate amount of time being spent in the construction of `_build_gate_lengths_by_qubit` and `_build_gate_errors_by_qubit` which isn't being used because this happens internally in rust now. To mitigate this overhead this commit migrates the sections computing these values to the code branch that uses them and not the default plugin branch that uses rust. --- .../passes/synthesis/unitary_synthesis.py | 70 ++++++++++--------- 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/qiskit/transpiler/passes/synthesis/unitary_synthesis.py b/qiskit/transpiler/passes/synthesis/unitary_synthesis.py index cf9acdc890c2..a2bd044c7341 100644 --- a/qiskit/transpiler/passes/synthesis/unitary_synthesis.py +++ b/qiskit/transpiler/passes/synthesis/unitary_synthesis.py @@ -457,37 +457,6 @@ def run(self, dag: DAGCircuit) -> DAGCircuit: default_kwargs = {} method_list = [(plugin_method, plugin_kwargs), (default_method, default_kwargs)] - for method, kwargs in method_list: - if method.supports_basis_gates: - kwargs["basis_gates"] = self._basis_gates - if method.supports_natural_direction: - kwargs["natural_direction"] = self._natural_direction - if method.supports_pulse_optimize: - kwargs["pulse_optimize"] = self._pulse_optimize - if method.supports_gate_lengths: - _gate_lengths = _gate_lengths or _build_gate_lengths( - self._backend_props, self._target - ) - kwargs["gate_lengths"] = _gate_lengths - if method.supports_gate_errors: - _gate_errors = _gate_errors or _build_gate_errors(self._backend_props, self._target) - kwargs["gate_errors"] = _gate_errors - if method.supports_gate_lengths_by_qubit: - _gate_lengths_by_qubit = _gate_lengths_by_qubit or _build_gate_lengths_by_qubit( - self._backend_props, self._target - ) - kwargs["gate_lengths_by_qubit"] = _gate_lengths_by_qubit - if method.supports_gate_errors_by_qubit: - _gate_errors_by_qubit = _gate_errors_by_qubit or _build_gate_errors_by_qubit( - self._backend_props, self._target - ) - kwargs["gate_errors_by_qubit"] = _gate_errors_by_qubit - supported_bases = method.supported_bases - if supported_bases is not None: - kwargs["matched_basis"] = _choose_bases(self._basis_gates, supported_bases) - if method.supports_target: - kwargs["target"] = self._target - # Handle approximation degree as a special case for backwards compatibility, it's # not part of the plugin interface and only something needed for the default # pass. @@ -503,7 +472,7 @@ def run(self, dag: DAGCircuit) -> DAGCircuit: else {} ) - if self.method == "default" and isinstance(kwargs["target"], Target): + if self.method == "default" and self._target is not None: _coupling_edges = ( set(self._coupling_map.get_edges()) if self._coupling_map is not None else set() ) @@ -512,13 +481,46 @@ def run(self, dag: DAGCircuit) -> DAGCircuit: dag, list(qubit_indices.values()), self._min_qubits, - kwargs["target"], + self._target, _coupling_edges, self._approximation_degree, - kwargs["natural_direction"], + self._natural_direction, ) return out else: + for method, kwargs in method_list: + if method.supports_basis_gates: + kwargs["basis_gates"] = self._basis_gates + if method.supports_natural_direction: + kwargs["natural_direction"] = self._natural_direction + if method.supports_pulse_optimize: + kwargs["pulse_optimize"] = self._pulse_optimize + if method.supports_gate_lengths: + _gate_lengths = _gate_lengths or _build_gate_lengths( + self._backend_props, self._target + ) + kwargs["gate_lengths"] = _gate_lengths + if method.supports_gate_errors: + _gate_errors = _gate_errors or _build_gate_errors( + self._backend_props, self._target + ) + kwargs["gate_errors"] = _gate_errors + if method.supports_gate_lengths_by_qubit: + _gate_lengths_by_qubit = _gate_lengths_by_qubit or _build_gate_lengths_by_qubit( + self._backend_props, self._target + ) + kwargs["gate_lengths_by_qubit"] = _gate_lengths_by_qubit + if method.supports_gate_errors_by_qubit: + _gate_errors_by_qubit = _gate_errors_by_qubit or _build_gate_errors_by_qubit( + self._backend_props, self._target + ) + kwargs["gate_errors_by_qubit"] = _gate_errors_by_qubit + supported_bases = method.supported_bases + if supported_bases is not None: + kwargs["matched_basis"] = _choose_bases(self._basis_gates, supported_bases) + if method.supports_target: + kwargs["target"] = self._target + out = self._run_main_loop( dag, qubit_indices, plugin_method, plugin_kwargs, default_method, default_kwargs )