Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leverage rustworkx node and edge count hints in transpiler #12812

Closed
wants to merge 3 commits into from

Conversation

mtreinish
Copy link
Member

Summary

This commit updates the DAGCircuit construction in the transpiler to use the newly introduced node_count_hint and edge_count_hint kwargs on the rustworkx graph constructors. In situations where we know the size of the dag or at least the lower bound we can reduce the number of underlying memory allocations by preallocating that size when we create the inner graph object. This commit adds private arguments to the DAGCircuit constructor which are used to provide these size hints to rustworkx.

Details and comments

This commit updates the DAGCircuit construction in the transpiler to use
the newly introduced node_count_hint and edge_count_hint kwargs on the
rustworkx graph constructors. In situations where we know the size of
the dag or at least the lower bound we can reduce the number of
underlying memory allocations by preallocating that size when we create
the inner graph object. This commit adds private arguments to the
DAGCircuit constructor which are used to provide these size hints to
rustworkx.
@mtreinish mtreinish added performance Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler labels Jul 24, 2024
@mtreinish mtreinish added this to the 1.2.0 milestone Jul 24, 2024
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @levbishop

@coveralls
Copy link

coveralls commented Jul 24, 2024

Pull Request Test Coverage Report for Build 10150963487

Details

  • 26 of 27 (96.3%) changed or added relevant lines in 8 files are covered.
  • 15 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.008%) to 89.767%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/synthesis/one_qubit/one_qubit_decompose.py 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.39%
crates/qasm2/src/lex.rs 6 92.37%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 7 90.84%
Totals Coverage Status
Change from base Build 10150038699: -0.008%
Covered Lines: 66971
Relevant Lines: 74605

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of count hints looks straightforward to me. I would have to look into the passes better to fully get where some of the counts are coming from, but I guess that there isn't any big issue if the tests pass.

Comment on lines 115 to 116
_node_count_hint=3 * num_qubits,
_edge_count_hint=2 * num_qubits,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usual question for me, how did you reach a node count (hint) of 3*num_qubits here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dagcircuit being built here is taking a n qubit barrier and splitting it into n 1 qubit barriers so we can compute the connected components of the circuit. So if it's a 3q barrier it would looks something like:

dag_barrier

So there are 3 nodes for every qubit and 2 edges.

@mtreinish mtreinish force-pushed the pre-allocate-dag-copy branch from 85028d5 to 5da0be5 Compare July 29, 2024 19:59
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this seems like a good idea. There's a few places where the amount of calculation / private-method accessing we're doing is sufficiently high that it might not be worthwhile in Python space, but I don't feel strongly.

The hints in copy_empty_like feel a bit more awkward - they make sense in in the context of copy or if the DAG will be refilled, but for things like layers, the overallocation feels like it could get quite out-of-hand.

Comment on lines +702 to 708
target_dag = DAGCircuit(
node_count_hint=self._multi_graph.num_nodes(),
edge_count_hint=self._multi_graph.num_edges(),
)
target_dag.name = self.name
target_dag._global_phase = self._global_phase
target_dag.duration = self.duration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should copy_empty_like eagerly allocate to the full size? Feels like that could be overallocating a bit. Maybe we want to control it with a single flag?

Comment on lines +306 to +317
num_nodes = 2 * (len(physical_qubits) + dag.num_clbits() + dag.num_vars)
num_edges = 0
for component in components:
component_dag = component.dag
size = len(component_dag._multi_graph) - 2 * len(component_dag._wires)
num_nodes += size
num_edges += component_dag._multi_graph.num_edges()

mapped_dag = DAGCircuit(
node_count_hint=num_nodes,
edge_count_hint=num_edges,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm suspicious that the Python-space time to calculate this stuff is quite possibly similar to the Rust-space time to just grow the vectors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was worried about this too. In practice I think the list is only ever a single list entry here though. The code handles disjoint connectivity, but iirc it was unsound to do the routing split up so we bail return an unrouted circuit before this gets called if the number of components is >1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to just leave it anyway - it's probably just going to be a wash at worst, rather than hurting.

@mtreinish
Copy link
Member Author

I'm going to close this as it's no longer relevant since we've moved the DAGCircuit to rust. There might still be places we want to do this from python but the implementation will have to be added as part of the rust implementation of the dag now.

@mtreinish mtreinish closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants