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

Overhead of SingletonGate.__new__ #10867

Closed
mtreinish opened this issue Sep 20, 2023 · 0 comments · Fixed by #11014
Closed

Overhead of SingletonGate.__new__ #10867

mtreinish opened this issue Sep 20, 2023 · 0 comments · Fixed by #11014
Assignees
Milestone

Comments

@mtreinish
Copy link
Member

In #10314 we introduced a new SingletonGate class which is used as the parent for many standard library gates. This class enables us to use a single shared instance for these gates which greatly reduces our memory overhead for repeated gates in a circuit. However, the manner in which implemented this object reuse is by overloading __new__ in the SingletonGate class which determines if the instance should be a singleton or a mutable copy and returns that. However, using __new__ has noticeable impact on runtime for creating new objects. @jakelishman outlined some of the reasons for this overhead in his comment on #10865:

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.

Originally posted by @jakelishman in #10865 (comment)

This overhead for object creation goes from previously being ~666ns for a class directly inheriting from Gate to ~5us changing the parent to SingletonGate. We should investigate different techniques to mitigate the runtime impact here.

@mtreinish mtreinish added this to the 0.45.0 milestone Sep 20, 2023
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Oct 6, 2023

Verified

This commit was signed with the committer’s verified signature.
mtreinish Matthew Treinish
This commit is a small optimization inside the
Optimize1qGatesDecomposition pass. The last stage of the pass is taking
a circuit sequence and using it to construct an equivalent 1q dag. To do
this the pass iterates over the returned circuit sequence from the 1q
synthesis routine and looks up the gate name in a mapping to gate
classes, and creates a new object of that class with any angles
provided. However, for XGate and SXGate there are no angles, and
since Qiskit#10314 merged there is extra overhead with the repeated
construction of these gate classes (see Qiskit#10867 for more details). Since
these gates are now singletons since Qiskit#10314 it is safe to just reuse the
same instance because calling XGate() will return that instance anyway.
This commit updates the DAGCircuit construction to just reuse the same
instance if the gate in circuit sequence is for x or sx.
@jakelishman jakelishman self-assigned this Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants