Make Optimize1qGatesDecomposition multithreaded#15567
Conversation
This commit switches to make optimize1qgatesdecomposition a parallel transpiler pass. After we collect the 1q runs in the dag the step of computing the unitary matrix for each run and synthesizing it has no data dependency and can be run in parallel without issue. However updating the dag with synthesis results is still serial so there are limits to how much can be parallelized here. Additionally, in the previous serial version the target euler bases to try were computed eagerly the first time a qubit with a run was encountered. This would force a data dependency between the threads which either require locking or precomputing the target bases, which is what this commit does. This means that instantiating the pass object is slower and we're potentially doing more work up front than is strictly necessary. However this does have the advantage of being amortizable over multiple executions of the pass which before it was not. A quick experiment was run to determine that when there are roughly 100,000 runs to process (all runs of 20 gates) is the crossover point for the parallel version vs the serial version. This was used to set a run count that is used to select between a serial and parallel version of the algorithm.
mtreinish
added
performance
Rust
This commit reworks the change in logic from the previous commit to no longer pre-compute the euler basis set for each qubit regardless of whether it's used or not. The state object used to store the basis gates and euler basis sets is kept as this enables more efficient patterns on multiple runs of the pass. Now the state uses OnceLock to enable each thread to lazily populate the state on the first run of a qubit. This saves the construction time overhead if qubits never have runs but keeps the advantages of reused state.
In earlier commits a crossover value of 100,000 runs was used to switch between serial and parallel runs. This was based on a scaling experiment that indicated this was about when parallel became faster. But further testing is showing this not to be as clear cut. Until we make a determination around that and finalize the implementation this commit leaves the value there as a TODO and the pass is always multithreaded unless in a multiprocessing context.
In the earlier commit moving to use lazy initialization this wasn't tested in a parallel context previously and the method of initialization wasn't atomic which led to a race condition between threads trying to populate runs on the same qubit concurrently. This commit fixes this by adjusting the OnceLock usage to properly use the API for initialization to fix this issue.
Coverage Report for CI Build 26915815564Coverage decreased (-0.03%) to 87.437%Details
Uncovered Changes
Coverage Regressions14 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
One or more of the following people are relevant to this code:
|
ASV benchmarks are flagging regressions which means this value is too large still. For now lets just remove this.
jakelishman
added
Changelog: Performance
| .all(|x| matches!(x, Param::ParameterExpression(_))) | ||
| .operations() | ||
| .filter_map(|op| { | ||
| if op.operation.num_qubits() == 1 { |
There was a problem hiding this comment.
Don't you need to add the ParameterExpression filtter here?
There was a problem hiding this comment.
Looking at this more closely the check is a bit nuanced this is in the self.global path, this is a weird path where we're treating the target like a basis_gates list where we don't have any of the finer grained detail from the target. So this is just trying to find the names of the 1q gates in the target to use for that global list. It's basically building on demand the equivalent of global_decomposers before this PR.
|
Does it need a performance release notes? |
Added in: fa636f8 |
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
I've gone through the code and it looks very self-explanatory. I left a couple of comments and suggestions. That said, I haven't been able to test the performance improvements myself. We did have an internal discussion of trying to find the right metric to benchmark. I'm still looking into that.
…-rust-with-rayon-multithreading-a108d4e761cb4429.yaml Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
This commit updates the way that the basis_gates_per_qubit field in the Optimize1qGatesDecompositionState is tracked. Previously, it was done using a `Option<HashSet<String>>` inside a OnceLock. This moves to using an explicit enum that is basically an Option<> but has named variants All and Gates. This provides descriptive typing for the variants. But more importantly it simplifies the typing around Python pickle serialization. Previously we were modeling the difference between None and OnceLock not populated as `Option<Option<HashSet<String>>>` as the type returned to PyO3 for pickle serialization. However, this was both an odd pattern in rust and potentially buggy in loading from Python (as the inner `None` case would get flattened into the outer Option). By moving to a custom enum with it's own PyO3 conversion trait implementations avoids this issue.
This was actually a bit more specific, it was about determining the scaling characteristics of running parallel vs running serially. If you look at this commit from the PR branch: 8f03254 what I was reverting there was my most recent attempt at what I was talking about offline. The idea is that for small circuits the parallelism adds overhead and is slightly slower than running serially. What I am having trouble with is defining a heuristic crossover value that is fast to compute and check and then use that to select between parallel and serial execution. My gut feeling was this should have been based on the number of gates or the number of 1q runs detected in the circuit, but neither of those was proving fruitful as having a clear cut threshold value. So I opted to just always run in parallel as the regression for small circuits is very small. The general benchmarking of this PR is fairly clear that it's just faster most of the time. I just realized I didn't publish the asv numbers, I'll kick off a new run and comment here with them. |
7 participants
Summary
This commit switches to make optimize1qgatesdecomposition a parallel transpiler pass. After we collect the 1q runs in the dag the step of computing the unitary matrix for each run and synthesizing it has no data dependency and can be run in parallel without issue. However updating the dag with synthesis results is still serial so there are limits to how much can be parallelized here. Additionally, in the previous serial version the target euler bases to try were computed eagerly the first time a qubit with a run was encountered. This would force a data dependency between the threads which either require locking or precomputing the target bases, which is what this commit does. This means that instantiating the pass object is slower and we're potentially doing more work up front than is strictly necessary. However this does have the advantage of being amortizable over multiple executions of the pass which before it was not.
A quick experiment was run to determine that when there are roughly 100,000 runs to process (all runs of 20 gates) is the crossover point for the parallel version vs the serial version. This was used to set a run count that is used to select between a serial and parallel version of the algorithm.
Details and comments
TODO: