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

Fix performance regression in CSPLayout pass #5514

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Dec 11, 2020

Summary

In #5183 the inner graph object of the CouplingMap object was switched
to retworkx. While generally faster there are a few different edge cases
where retworkx performance can be worse than networkx. Namely, any place
where large lists are returned, for example lists of all nodes or edges
in a graph. In those cases the conversion from the Rust objects to the
Python objects can cumulatively be expensive. To avoid this conversion
overhead in every case retworkx has custom return types that are python
sequences but defer type conversion to __getitem__, meaning that the
return is fast but complete traversal can be slow, especially when done
multiple times. In such cases it's easier to cast the sequence to a
Python object such as a set or a list so the conversion is only done
once. However, in the CSPLayout pass the edges list returned from
retworkx was searched multiple times as part of the constraint function
which resulted in a large performance regression. This commit fixes this
by casting the local edges to a set(). This has 2 advantages, first it
avoids the multiple traversals of the graph which removes the conversion
overhead. The second advantage is that by using a set the lookups are
constant time instead of iterating over the full list every time. This
results in a performance improvement over the performance prior to #5183
and the introduction of the regression.

Details and comments

In Qiskit#5183 the inner graph object of the CouplingMap object was switched
to retworkx. While generally faster there are a few different edge cases
where retworkx performance can be worse than networkx. Namely, any place
where large lists are returned, for example lists of all nodes or edges
in a graph. In those cases the conversion from the Rust objects to the
Python objects can culmulatively be expensive. To avoid this conversion
overhead in every case retworkx has custom return types that are python
sequences but defer type conversion to __getitem__, meaning that the
return is fast but complete traversal can be slow, epsecially when done
multiple times. In such cases it's easier to cast the sequence to a
Python object such as a set or a list so the conversion is only done
once. However, in the CSPLayout pass the edges list returned from
retworkx was searched multiple times as part of the constraint function
which resulted in a large performance regression. This commit fixes this
by casting the local edges to a set(). This has 2 advantages, first it
avoids the multiple traversals of the graph which removes the conversion
overhead. The second advantage is that by using a set the lookups are
constant time instead of iterating over the full list every time. This
results in a performance improvement over the performance prior to Qiskit#5183
and the introduction of the regression.
@mtreinish mtreinish requested a review from a team as a code owner December 11, 2020 19:40
@mtreinish mtreinish added this to the 0.17 milestone Dec 11, 2020
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Looks good. Should other callers of get_edges be similarly updated (or should this be handled in the CouplingMap class)?

@mtreinish
Copy link
Member Author

Looks good. Should other callers of get_edges be similarly updated (or should this be handled in the CouplingMap class)?

It depends on the access pattern, in general we probably don't want to cast the output from get_edges() in CouplingMap because for single item access or single iteration the EdgeList return is fast enough (and we want to avoid the conversion overhead unless something is actually using the output). But if there are places that are doing multiple lookups like here or multiple iterations over the output we probably want to cast to a set or a list in those places.

@kdk kdk merged commit 6646e68 into Qiskit:master Dec 11, 2020
@mtreinish mtreinish deleted the csp-regression branch December 12, 2020 00:24
@kdk kdk added the Changelog: None Do not include in changelog label Mar 31, 2021
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 changelog performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants