Skip to content

Avoid clone on entry to BasisTranslator#14761

Merged
mtreinish merged 1 commit into
Qiskit:mainfrom
jakelishman:basis-translator-ownership
Jul 19, 2025
Merged

Avoid clone on entry to BasisTranslator#14761
mtreinish merged 1 commit into
Qiskit:mainfrom
jakelishman:basis-translator-ownership

Conversation

@jakelishman

Copy link
Copy Markdown
Member

The Rust-space method takes ownership of the DAGCircuit so that the return type can be a unified PyResult<DAGCircuit>, even though in the majority case, the pass actually builds a completely new DAG. This unfortunately requires an implicit clone on entry from Python space, even though there is no situation where the clone is actually necessary; the rebuild form never mutates the input, and in the fast-return paths, Python space can simply continue using the reference to a DAGCircuit it already holds.

Summary

Details and comments

The Rust-space method takes ownership of the `DAGCircuit` so that the
return type can be a unified `PyResult<DAGCircuit>`, even though in the
majority case, the pass actually builds a completely new DAG.  This
unfortunately requires an implicit clone on entry from Python space,
even though there is no situation where the clone is actually
_necessary_; the rebuild form never mutates the input, and in the
fast-return paths, Python space can simply continue using the reference
to a `DAGCircuit` it already holds.
@jakelishman jakelishman added this to the 2.2.0 milestone Jul 19, 2025
@jakelishman jakelishman requested a review from a team as a code owner July 19, 2025 00:48
@jakelishman jakelishman added performance Changelog: None Do not include in the GitHub Release changelog. Rust This PR or issue is related to Rust code in the repository labels Jul 19, 2025
@qiskit-bot

Copy link
Copy Markdown
Collaborator

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

  • @Qiskit/terra-core

@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 16383075235

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.009%) to 87.775%

Files with Coverage Reduction New Missed Lines %
crates/circuit/src/symbol_expr.rs 3 73.73%
crates/qasm2/src/lex.rs 5 92.01%
Totals Coverage Status
Change from base Build 16327707788: 0.009%
Covered Lines: 81485
Relevant Lines: 92834

💛 - Coveralls

@mtreinish mtreinish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This signature is also more conducive for #14760 so we were likely to need it one way or another.

@mtreinish mtreinish added this pull request to the merge queue Jul 19, 2025
Merged via the queue into Qiskit:main with commit 9aa1477 Jul 19, 2025
27 checks passed
@jakelishman jakelishman deleted the basis-translator-ownership branch July 19, 2025 12:56
raynelfss pushed a commit to raynelfss/qiskit that referenced this pull request Aug 1, 2025
The Rust-space method takes ownership of the `DAGCircuit` so that the
return type can be a unified `PyResult<DAGCircuit>`, even though in the
majority case, the pass actually builds a completely new DAG.  This
unfortunately requires an implicit clone on entry from Python space,
even though there is no situation where the clone is actually
_necessary_; the rebuild form never mutates the input, and in the
fast-return paths, Python space can simply continue using the reference
to a `DAGCircuit` it already holds.
raynelfss pushed a commit to raynelfss/qiskit that referenced this pull request Aug 1, 2025
The Rust-space method takes ownership of the `DAGCircuit` so that the
return type can be a unified `PyResult<DAGCircuit>`, even though in the
majority case, the pass actually builds a completely new DAG.  This
unfortunately requires an implicit clone on entry from Python space,
even though there is no situation where the clone is actually
_necessary_; the rebuild form never mutates the input, and in the
fast-return paths, Python space can simply continue using the reference
to a `DAGCircuit` it already holds.
raynelfss pushed a commit to raynelfss/qiskit that referenced this pull request Aug 4, 2025
The Rust-space method takes ownership of the `DAGCircuit` so that the
return type can be a unified `PyResult<DAGCircuit>`, even though in the
majority case, the pass actually builds a completely new DAG.  This
unfortunately requires an implicit clone on entry from Python space,
even though there is no situation where the clone is actually
_necessary_; the rebuild form never mutates the input, and in the
fast-return paths, Python space can simply continue using the reference
to a `DAGCircuit` it already holds.
aaryav-3 pushed a commit to aaryav-3/qiskit that referenced this pull request Oct 21, 2025
The Rust-space method takes ownership of the `DAGCircuit` so that the
return type can be a unified `PyResult<DAGCircuit>`, even though in the
majority case, the pass actually builds a completely new DAG.  This
unfortunately requires an implicit clone on entry from Python space,
even though there is no situation where the clone is actually
_necessary_; the rebuild form never mutates the input, and in the
fast-return paths, Python space can simply continue using the reference
to a `DAGCircuit` it already holds.
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 the GitHub Release changelog. performance Rust This PR or issue is related to Rust code in the repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants