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

Use singleton SwapGate in Sabre reconstruction #10865

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

jakelishman
Copy link
Member

Summary

Since SwapGate is now a singleton instance in most cases, we can directly reuse the same instance during Sabre reconstruction rather than wasting cycles re-retrieving the singleton instance.

Details and comments

See #10314 for the root of this.

@jakelishman jakelishman added performance Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler labels Sep 20, 2023
@jakelishman jakelishman added this to the 0.45.0 milestone Sep 20, 2023
@jakelishman jakelishman requested a review from a team as a code owner September 20, 2023 12:34
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@alexanderivrii
Copy link
Contributor

With #10314 merged, both swap_singleton and SwapGate() return the same shared singleton instance, right? How much of a performance improvement does this give?

@mtreinish
Copy link
Member

In my local testing the overhead of calling SwapGate() is ~5us per call:

In [4]: %timeit qiskit.circuit.library.SwapGate()
5.06 µs ± 72.1 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

with #10314. This is actually a significant regression because before #10314 it took ~666ns:

In [3]: %timeit qiskit.circuit.library.SwapGate()
666 ns ± 2.18 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

so doing this can save a noticeable amount of time depending on how many swaps we need to create.

mtreinish
mtreinish previously approved these changes Sep 20, 2023
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, it's a straightforward optimization. We should look at the runtime overhead of SingletonGate.__new__ in parallel to try and minimize the overhead there.

@jakelishman
Copy link
Member Author

Fwiw, there's a bunch of "hidden" overhead of SingletonGate.__new__ in that:

  • making the __new__ an extra Python-space call over object.__new__ adds cost of itself
  • even if we return the singleton instance from __new__, the resulting __init__ still gets called if the type of return value is in the inheritance chain of the original object.

One potential way around this is to have the singleton class be a metaclass that overrides type.__call__ with a new method that returns the singleton instance without calling the __new__ machinery at all. That will avoid __init__ from being called at all, so we'll gain really a bunch of the time there. The downside is that there's a metaclass conflict: Operation's metaclass is ABCMeta, so we can't (naively) make SingletonGate a metaclass. However, we could make SingletonGateMeta derive from ABCMeta, which would resolve the conflict, even if we'd need to take care to maintain that relationship were Operation ever to change.

Since `SwapGate` is now a singleton instance in most cases, we can
directly reuse the same instance during Sabre reconstruction rather than
wasting cycles re-retrieving the singleton instance.
@jakelishman
Copy link
Member Author

Now rebased over #10782.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6250595529

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 87.279%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 5 91.92%
Totals Coverage Status
Change from base Build 6250386385: 0.03%
Covered Lines: 74337
Relevant Lines: 85172

💛 - Coveralls

@mtreinish mtreinish added this pull request to the merge queue Sep 20, 2023
Merged via the queue into Qiskit:main with commit 82aaef5 Sep 20, 2023
@jakelishman jakelishman deleted the sabre/singleton-reuse branch September 20, 2023 19:52
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 mod: transpiler Issues and PRs related to Transpiler performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants