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

Runtime Performance Regression in SabreLayout #10650

Closed
mtreinish opened this issue Aug 16, 2023 · 1 comment · Fixed by #10652
Closed

Runtime Performance Regression in SabreLayout #10650

mtreinish opened this issue Aug 16, 2023 · 1 comment · Fixed by #10652
Labels
Milestone

Comments

@mtreinish
Copy link
Member

Environment

  • Qiskit Terra version: 0.25.0
  • Python version: 3.11
  • Operating system: Linux

What is happening?

When running SabreLayout with 0.25.0 has a large performance regression between 0.24.x and 0.25.0 that is a function of the number of gates in the circuit. The rust computation of the swap map and initial layout is as fast as before, but when we iterate over that to rewrite the DAGCircuit using that output from rust the runtime performance is significantly slower. For more modestly sized circuits this isn't super noticeable , but as the number of instructions in a circuit increases this overhead can be significantly higher.

How can we reproduce the issue?

You can parse this qasm file from qasmbench: https://github.com/pnnl/QASMBench/blob/master/medium/bwt_n21/bwt_n21.qasm

from qiskit import transpile, QuantumCircuit
from qiskit.providers.fake_provider import FakeCairoV2
import time
import logging
logging.basicConfig(level="INFO")

backend = FakeCairoV2()
for filename in ["bwt_n21.qasm"]:
    start = time.time()
    qc = QuantumCircuit.from_qasm_file(filename)
    parse_stop = time.time()
    print(f"parse {filename}: {parse_stop - start}")
    tqc = transpile(qc, backend, optimization_level=1)
    stop = time.time()
    print(f"{filename}: {stop - start}")

What should happen?

The performance should be roughly equivalent to qiskit-terra < 0.25.0. Looking at 0.24.2 the above example SabreLayout took 22515.55800 ms and 0.23.0 took 18759.72962 ms. But I've been too impatient to wait for 0.25.0 to finish after several minutes.

Any suggestions?

I expect the underlying issue stems from #10366 and specifically the rust<->python boundary doing unnecessary copies or conversions when we access data as we iterate over the circuit.

@mtreinish mtreinish added bug Something isn't working priority: high performance labels Aug 16, 2023
@mtreinish mtreinish added this to the 0.25.1 milestone Aug 16, 2023
@mtreinish
Copy link
Member Author

The root cause of this seems to be because of this line: https://github.com/Qiskit/qiskit-terra/blob/main/crates/accelerate/src/sabre_swap/mod.rs#L67 implementing a getter with the attribute macro like this results in map being cloned on each access of SabreResult.map. So we're basically doing a clone() of the full swap map for each node in the dag as we iterate over the output.

mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Aug 16, 2023
In Qiskit#10366 the SabreLayout and SabreSwap passes were refactored to
support nested control flow blocks. As part of that refactor a new
struct SabreResult was created to store the nested results for each
block. This new class however resulted in PyO3 cloning the full swap map
on every access. Since we have at least 1 access per gate in the circuit
(and another one for each swap) this extra clone() adds a lot of extra
overhead for deep circuits. In extreme cases this regression could be
quite extreme. To address this the result format is changed to be a
tuple (as it was originally), while this is less ergonomic the advantage
it provides is that for nested objects it moves the rust object to the
pyo3 interface so we avoid a copy as Python owns the object on the
return. However, for control flow blocks we're still using the
SabreResult class as it simplifies the internal implementation (which is
why Qiskit#10366 introduced the struct). The potential impact of this is
mitigated by switching to only a single clone per control flow block,
by only accessing the SabreResult object's attribute on each recursive
call. However, if it is an issue in the future we can work on flattening
the nested structure before returning it to python to avoid any clones.

Fixes Qiskit#10650
github-merge-queue bot pushed a commit that referenced this issue Aug 17, 2023
* Fix performance of Sabre rust<->Python boundary

In #10366 the SabreLayout and SabreSwap passes were refactored to
support nested control flow blocks. As part of that refactor a new
struct SabreResult was created to store the nested results for each
block. This new class however resulted in PyO3 cloning the full swap map
on every access. Since we have at least 1 access per gate in the circuit
(and another one for each swap) this extra clone() adds a lot of extra
overhead for deep circuits. In extreme cases this regression could be
quite extreme. To address this the result format is changed to be a
tuple (as it was originally), while this is less ergonomic the advantage
it provides is that for nested objects it moves the rust object to the
pyo3 interface so we avoid a copy as Python owns the object on the
return. However, for control flow blocks we're still using the
SabreResult class as it simplifies the internal implementation (which is
why #10366 introduced the struct). The potential impact of this is
mitigated by switching to only a single clone per control flow block,
by only accessing the SabreResult object's attribute on each recursive
call. However, if it is an issue in the future we can work on flattening
the nested structure before returning it to python to avoid any clones.

Fixes #10650

* Simplify recursive call logic in _apply_sabre_result

This commit simplifies the logic in the recursive call logic in
_apply_sabre_result to always use a tuple so we avoid an isinstance
check.

Co-authored-by: Kevin Hartman <kevin@hart.mn>

---------

Co-authored-by: Kevin Hartman <kevin@hart.mn>
mergify bot pushed a commit that referenced this issue Aug 17, 2023
* Fix performance of Sabre rust<->Python boundary

In #10366 the SabreLayout and SabreSwap passes were refactored to
support nested control flow blocks. As part of that refactor a new
struct SabreResult was created to store the nested results for each
block. This new class however resulted in PyO3 cloning the full swap map
on every access. Since we have at least 1 access per gate in the circuit
(and another one for each swap) this extra clone() adds a lot of extra
overhead for deep circuits. In extreme cases this regression could be
quite extreme. To address this the result format is changed to be a
tuple (as it was originally), while this is less ergonomic the advantage
it provides is that for nested objects it moves the rust object to the
pyo3 interface so we avoid a copy as Python owns the object on the
return. However, for control flow blocks we're still using the
SabreResult class as it simplifies the internal implementation (which is
why #10366 introduced the struct). The potential impact of this is
mitigated by switching to only a single clone per control flow block,
by only accessing the SabreResult object's attribute on each recursive
call. However, if it is an issue in the future we can work on flattening
the nested structure before returning it to python to avoid any clones.

Fixes #10650

* Simplify recursive call logic in _apply_sabre_result

This commit simplifies the logic in the recursive call logic in
_apply_sabre_result to always use a tuple so we avoid an isinstance
check.

Co-authored-by: Kevin Hartman <kevin@hart.mn>

---------

Co-authored-by: Kevin Hartman <kevin@hart.mn>
(cherry picked from commit e6c431e)
github-merge-queue bot pushed a commit that referenced this issue Aug 17, 2023
* Fix performance of Sabre rust<->Python boundary

In #10366 the SabreLayout and SabreSwap passes were refactored to
support nested control flow blocks. As part of that refactor a new
struct SabreResult was created to store the nested results for each
block. This new class however resulted in PyO3 cloning the full swap map
on every access. Since we have at least 1 access per gate in the circuit
(and another one for each swap) this extra clone() adds a lot of extra
overhead for deep circuits. In extreme cases this regression could be
quite extreme. To address this the result format is changed to be a
tuple (as it was originally), while this is less ergonomic the advantage
it provides is that for nested objects it moves the rust object to the
pyo3 interface so we avoid a copy as Python owns the object on the
return. However, for control flow blocks we're still using the
SabreResult class as it simplifies the internal implementation (which is
why #10366 introduced the struct). The potential impact of this is
mitigated by switching to only a single clone per control flow block,
by only accessing the SabreResult object's attribute on each recursive
call. However, if it is an issue in the future we can work on flattening
the nested structure before returning it to python to avoid any clones.

Fixes #10650

* Simplify recursive call logic in _apply_sabre_result

This commit simplifies the logic in the recursive call logic in
_apply_sabre_result to always use a tuple so we avoid an isinstance
check.

Co-authored-by: Kevin Hartman <kevin@hart.mn>

---------

Co-authored-by: Kevin Hartman <kevin@hart.mn>
(cherry picked from commit e6c431e)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
SamD-1998 pushed a commit to SamD-1998/qiskit-terra that referenced this issue Sep 7, 2023
* Fix performance of Sabre rust<->Python boundary

In Qiskit#10366 the SabreLayout and SabreSwap passes were refactored to
support nested control flow blocks. As part of that refactor a new
struct SabreResult was created to store the nested results for each
block. This new class however resulted in PyO3 cloning the full swap map
on every access. Since we have at least 1 access per gate in the circuit
(and another one for each swap) this extra clone() adds a lot of extra
overhead for deep circuits. In extreme cases this regression could be
quite extreme. To address this the result format is changed to be a
tuple (as it was originally), while this is less ergonomic the advantage
it provides is that for nested objects it moves the rust object to the
pyo3 interface so we avoid a copy as Python owns the object on the
return. However, for control flow blocks we're still using the
SabreResult class as it simplifies the internal implementation (which is
why Qiskit#10366 introduced the struct). The potential impact of this is
mitigated by switching to only a single clone per control flow block,
by only accessing the SabreResult object's attribute on each recursive
call. However, if it is an issue in the future we can work on flattening
the nested structure before returning it to python to avoid any clones.

Fixes Qiskit#10650

* Simplify recursive call logic in _apply_sabre_result

This commit simplifies the logic in the recursive call logic in
_apply_sabre_result to always use a tuple so we avoid an isinstance
check.

Co-authored-by: Kevin Hartman <kevin@hart.mn>

---------

Co-authored-by: Kevin Hartman <kevin@hart.mn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant