Allow control-flow block extraction to correct type #15435
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes one of the largest requirements for Python interaction in the core data model. However, it currently comes at a major cost of cloning both on extraction from Python space and on producing view objects back _to_ Python space. We are choosing to make this trade-off as a temporary measure, to prioritise separation of the core data model from Python, given that transpiler performance in the presence of control flow is already poor. While `CircuitData` and `DAGCircuit` are both directly `pyclass`, we either have to encode the Python interaction deep into the data structures (deeply undesirable) to handle the blocks, or we have this current problematic cloning. The way around this will be to separate off `PyCircuitData(Ptr<CircuitData>)` and similar for `DAGCircuit`, where `Ptr<T>` is some smart-pointer type that may represent either ownership of an underlying object or a (maybe fallible) reference to an owned object. The exact mechanism of this (perhaps something `Arc`/`Weak`-based) is not yet decided, but will likely motivate a further change of the `blocks` structure. This commit is now a completely remade version of a previous PR (Qiskitgh-15123), but much of the set up work, investigation and planning of what became this was originally done by Kevin Hartman. Co-authored-by: Kevin Hartman <[email protected]>
Previously, the `OperationFromPython` extraction helper automatically chose `CircuitData` for the control-flow blocks. Since this extraction automatically required a clone anyway, the assumed type came at a cost. This allows `DAGCircuit` to directly extract blocks to the correct type without the intermediate step. As a side effect, we also allow extraction to fail with a type error if blocks are not expected, and add a way to leave blocks unextracted.
jakelishman
added
performance
Changelog: None
Collaborator
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 20117771897Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Previously, the
OperationFromPythonextraction helper automatically choseCircuitDatafor the control-flow blocks. Since this extraction automatically required a clone anyway, the assumed type came at a cost.This allows
DAGCircuitto directly extract blocks to the correct type without the intermediate step. As a side effect, we also allow extraction to fail with a type error if blocks are not expected, and add a way to leave blocks unextracted.Details and comments
Built on top of #15432. It's good for a ~10% improvement in the benchmark in that PR, which probably isn't worth worrying about too much.