-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fully port BarrierBeforeFinalMeasurements to rust #13220
Fully port BarrierBeforeFinalMeasurements to rust #13220
Conversation
This commit migrates the BarrierBeforeFinalMeasurements transpiler pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. The one exception is when the `label` is not set then we still call `MergeAdjacentBarriers` in the python code of the pass. This is the first step in the improvement of the performance of this pass. We can easily leverage multhreading to potentially parallelize the analysis portion of this pass that searches for the final operations and returns the set of indices. But this is blocked on Qiskit#13219 which prevents us from accessing the PackedInstructions stored in the DAGCircuit in a multithreaded context. This commit also fixes an issue related to shared references in the disjoint_utils module around barrier labels. The combine_barriers() function was incorrectly mutating the label by reference which wouldn't persist in the DAG, and this was causing failures after the barrier was originally generated in Rust with this pass now. Fixes Qiskit#12253
One or more of the following people are relevant to this code:
|
I ran the dedicated asv microbenchmarks on this and it yielded:
|
Pull Request Test Coverage Report for Build 11022176994Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking over this pass. It looks pretty straightforward, I only had one small question.
#[cfg(feature = "cache_pygates")] | ||
instruction: new_barrier.clone().unbind(), | ||
#[cfg(not(feature = "cache_pygates"))] | ||
instruction: new_barrier.unbind(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between doing this or simply omitting the #[cfg(feature = "cache_pygates")]
comment and setting the instruction argument to new_barrier.unbind()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When cache_pygates
is necessary we need to clone new_barrier
because it's used in the PyInstruction
here and also again in the pycache for the PackedInstruction
that we insert into the dag. I used the cfg macros here to conditionally compile this so we only clone if the cache_pygates
feature is enabled and otherwise skip the clone()
where it's not needed. This would work fine if I removed the macros and just had it always do new_barrier.clone().unbind()
regardless of whether the feature was enabled, but it was just me micro-optimizing a little as the clone()
and Drop
for new_barrier
as a Bound<PyAny>
is just python ref counting and very cheap, but I figured I could just save the overhead of that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
* Fully port BarrierBeforeFinalMeasurements to rust This commit migrates the BarrierBeforeFinalMeasurements transpiler pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. The one exception is when the `label` is not set then we still call `MergeAdjacentBarriers` in the python code of the pass. This is the first step in the improvement of the performance of this pass. We can easily leverage multhreading to potentially parallelize the analysis portion of this pass that searches for the final operations and returns the set of indices. But this is blocked on Qiskit#13219 which prevents us from accessing the PackedInstructions stored in the DAGCircuit in a multithreaded context. This commit also fixes an issue related to shared references in the disjoint_utils module around barrier labels. The combine_barriers() function was incorrectly mutating the label by reference which wouldn't persist in the DAG, and this was causing failures after the barrier was originally generated in Rust with this pass now. Fixes Qiskit#12253 * Remove unused imports
* Fully port BarrierBeforeFinalMeasurements to rust This commit migrates the BarrierBeforeFinalMeasurements transpiler pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. The one exception is when the `label` is not set then we still call `MergeAdjacentBarriers` in the python code of the pass. This is the first step in the improvement of the performance of this pass. We can easily leverage multhreading to potentially parallelize the analysis portion of this pass that searches for the final operations and returns the set of indices. But this is blocked on Qiskit#13219 which prevents us from accessing the PackedInstructions stored in the DAGCircuit in a multithreaded context. This commit also fixes an issue related to shared references in the disjoint_utils module around barrier labels. The combine_barriers() function was incorrectly mutating the label by reference which wouldn't persist in the DAG, and this was causing failures after the barrier was originally generated in Rust with this pass now. Fixes Qiskit#12253 * Remove unused imports
Summary
This commit migrates the BarrierBeforeFinalMeasurements transpiler pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. The one exception is when the
label
is not set then we still callMergeAdjacentBarriers
in the python code of the pass.This is the first step in the improvement of the performance of this pass. We can easily leverage multhreading to potentially parallelize the analysis portion of this pass that searches for the final operations and returns the set of indices. But this is blocked on #13219 which prevents us from accessing the PackedInstructions stored in the DAGCircuit in a multithreaded context.
This commit also fixes an issue related to shared references in the disjoint_utils module around barrier labels. The combine_barriers() function was incorrectly mutating the label by reference which wouldn't persist in the DAG, and this was causing failures after the barrier was originally generated in Rust with this pass now.
Details and comments
Fixes #12253