Cache Bit and Register __repr__ and use for __eq__ #5272
Conversation
This commit caches the generated repr for the Bit object (and its subtypes Qubit, Clbit, etc) at init time. Currently when we create DAGNode objects in the dagcircuit a big portion of the time is spent regenerating the str for the repr. However this information is static and the string manipulation which is slow doesn't need to be run constantly.
|
The main goal of this is to speed up
While with this PR:
(in both profiles the pink block is Where this comes up more is after #5267 merges, because then the |
This commit updates the __eq__ method of both Bit and Register to just compare the cached _repr value. The previous __eq__ methods were nested checks of various attributes, and while individually those are all fast when doing large list comprehensions the multiple checks in each function add up in runtime. All the information in the _repr attribute are identical to the multiple checks previously done in __eq__, so this reduces the __eq__ function to a single string comparison which in aggregate ends up being significantly faster.
mtreinish
force-pushed
the
cache_repr_bits
branch
from
8351066
to
061583b
Compare
mtreinish
changed the title
|
To continue the speed up after looking at profiles from #5267 (comment) I realized the next biggest time sync in |
mtreinish
added
the
stable backport potential
|
I just ran profiles of the same 65 by 4096 random circuit circuit to dag conversion again, and with
Then with this PR in it's current form (caching
|
mtreinish
removed
the
stable backport potential
|
I'm removing the stable backport tag here. I don't think the performance improvement here is really worth the risk of regression on the stable branch. It's more than I thought without #5267 but it doesn't really seem big enough to backport. |
In sabre_swap the comprehension in _score_heuristic() with the basic heuristic starts to become a bottleneck as the size of the coupling map increases. This probably becomes the top bottleneck in a transpilation after Qiskit#5316, Qiskit#5183, Qiskit#5294, Qiskit#5267, and Qiskit#5272 are merged. This commit is an attempt to try and mitigate that a bit by using numpy native operations instead of a python comprehension in sum(). If this is insufficient we'll likely have to either avoid doing a sum like this or drop down to a lower level with cython or rust and operate on the array directly to remove this bottleneck.
* Use numpy sum instead of comprehension in _score_heuristic In sabre_swap the comprehension in _score_heuristic() with the basic heuristic starts to become a bottleneck as the size of the coupling map increases. This probably becomes the top bottleneck in a transpilation after #5316, #5183, #5294, #5267, and #5272 are merged. This commit is an attempt to try and mitigate that a bit by using numpy native operations instead of a python comprehension in sum(). If this is insufficient we'll likely have to either avoid doing a sum like this or drop down to a lower level with cython or rust and operate on the array directly to remove this bottleneck. * Make distance matrix a coupling map property * Fix tests * Fix lint Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
In Qiskit#5272 the __eq__ method was changed to use a single string comparison away from a nested attribute comparison for performance reasons. This had the unexpected side effect that when comparing a register or a bit object against an object of another type an AttributeError would be raised because the private _repr attribute doesn't exist outside of those classes. This leads to unexpected failures when running with aqua because they were comparing a register to a string. This commit addresses this by catching the attribute error and returning false because objects of different types are not equal.
In #5272 the __eq__ method was changed to use a single string comparison away from a nested attribute comparison for performance reasons. This had the unexpected side effect that when comparing a register or a bit object against an object of another type an AttributeError would be raised because the private _repr attribute doesn't exist outside of those classes. This leads to unexpected failures when running with aqua because they were comparing a register to a string. This commit addresses this by catching the attribute error and returning false because objects of different types are not equal.
Summary
This commit caches the generated repr for the Bit class (and its
subtypes Qubit, Clbit, etc) and Register class at init time. Currently
when we create DAGNode objects in the dagcircuit a big portion of
the time is spent regenerating the str for the repr. However this
information is static and the string manipulation which is slow
doesn't need to be run constantly.
Details and comments