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

Fast vs safe attribute access #7035

Closed
georgios-ts opened this issue Sep 17, 2021 · 1 comment · Fixed by #7036
Closed

Fast vs safe attribute access #7035

georgios-ts opened this issue Sep 17, 2021 · 1 comment · Fixed by #7036
Labels
performance status: pending PR It has one or more PRs pending to solve this issue type: enhancement It's working, but needs polishing

Comments

@georgios-ts
Copy link
Contributor

What is the expected enhancement?

A profile of SabreSwap shows that spends most of the item accessing the layout item https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/layout.py#L100-L105 and the coupling map distance values https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/coupling.py#L176. These methods are fast but are called a huge number of times, so every tiny overhead occurred by a check starts accumulating. For example, transpiling a 100-qubit random circuit in a grid topology takes 388.71 sec. If instead sabre accesses directly the appropriate private fields of Layout and CouplingMap, the same code runs in 174.02 sec.

The issue here is to discuss appropriate solutions (e.g whether we should access private fields or provide unchecked access methods) and collect other performance critical places that share the same problem.

Details

The benchmark was to done using

import time
import cProfile

from qiskit import transpile
from qiskit.transpiler import CouplingMap
from qiskit.circuit.random import random_circuit

r = 10
coupling = CouplingMap().from_grid(r, r)

num = coupling.size()
qc = random_circuit(num, num, seed=42)

method = 'sabre'
start = time.time()
cProfile.run(
    "result = transpile( \
        qc, \
        coupling_map=coupling, \
        routing_method=method, \
        seed_transpiler=42, \
    )",
    filename='routing-{}.prof'.format(method)
)
stop = time.time()

print(stop - start)

The single change made was this line
https://github.com/Qiskit/qiskit-terra/blob/685622de76b1b9074f41f6a674e1bc15af8c3c3c/qiskit/transpiler/passes/routing/sabre_swap.py#L332
to

cost += int(
    self.coupling_map._dist_matrix[
        layout._v2p[node.qargs[0]], layout._v2p[node.qargs[1]]
    ]
)

The profile before
before
and after the "optimization"
optimized

@georgios-ts georgios-ts added the type: enhancement It's working, but needs polishing label Sep 17, 2021
@mtreinish
Copy link
Member

We had a similar issue in: #6493 the way we fixed it there was to re-architect things to provide a stable fast path api. In this case though I don't think we need to do a huge change with introducing new classes. The methods there now are nice for direct user facing things that people are manually interacting with, but that doesn't need to be speed optimized, but for transpiler passes we should support the fast access patterns so we don't waste a ton of time on pointless input validation, when we know the context.

I think for both layout and coupling map we should make the internal arrays public instead of private and then also slot the attributes of the class to improve access time further. Also for coupling map we should make compute_distance_matrix() public too.

mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Sep 17, 2021
This commit updates the layout and coupling map classes to tune them for
performance when used by transpiler passes. Right now the user facing
api is doing a bunch of extra validation to provide helpful user
messages and the internal data structures of both classes are hidden
behind this api. This can add a bunch of overhead to transpiler passes
(specifically routing passes) that are using these APIs in a loop. To
address this commit changes these internal properties to public
attributes and adds slots to the classes to speed up direct attribute
access. This will enable us to tune transpiler passes to access these
attributes more easily and faster. The follow on step here is to update
any transpiler passes using the user facing api to access the attributes
directly.

Fixes Qiskit#7035
@1ucian0 1ucian0 added the status: pending PR It has one or more PRs pending to solve this issue label Sep 21, 2021
@mergify mergify bot closed this as completed in #7036 Nov 19, 2021
mergify bot pushed a commit that referenced this issue Nov 19, 2021
…for fast access (#7036)

* Make internal layout and coupling map attributes public and slotted

This commit updates the layout and coupling map classes to tune them for
performance when used by transpiler passes. Right now the user facing
api is doing a bunch of extra validation to provide helpful user
messages and the internal data structures of both classes are hidden
behind this api. This can add a bunch of overhead to transpiler passes
(specifically routing passes) that are using these APIs in a loop. To
address this commit changes these internal properties to public
attributes and adds slots to the classes to speed up direct attribute
access. This will enable us to tune transpiler passes to access these
attributes more easily and faster. The follow on step here is to update
any transpiler passes using the user facing api to access the attributes
directly.

Fixes #7035

* Update pass access patterns for performance

* Move connectivity check inside distance matrix rebuild condition

* Adjust array access pattern to avoid temporary object contruction

* Make attributes private again

* Update sabre layout virtual bit usage

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <[email protected]>

* Get mapping from layout once outside of loop

This commit updates the layout usage in ApplyLayout and Layout2QDistance
to access the internal map once and store that in a local variable which
is then accessed from the loop using the layout.

Co-authored-by: Kevin Krsulich <[email protected]>

* Update qiskit/transpiler/passes/layout/sabre_layout.py

Co-authored-by: Jake Lishman <[email protected]>

* Locally cache objects in compute_cost

* Adjust access in full ancilla and layout2q passes to avoid private access

* Adjust gate_direction to limit private access

* Add comments about private access to Checkmap

* Update qiskit/transpiler/passes/layout/layout_2q_distance.py

Co-authored-by: Kevin Krsulich <[email protected]>

* Updates to sabre passes

* Remove unecessary private access from stochastic swap

* Add comment on private access to stochastic swap

* Adjust distance matrix access

Co-authored-by: Kevin Krsulich <[email protected]>
Co-authored-by: Jake Lishman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance status: pending PR It has one or more PRs pending to solve this issue type: enhancement It's working, but needs polishing
Projects
None yet
3 participants