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

Speed up hashing for GridQubit, LineQubit, and NamedQubit #6350

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Nov 17, 2023

This speeds up hashing on GridQubit, GridQid, LineQubit, and LineQid. Instead of using _compat.cached_method, we store an explicit self._hash property to cache computing the hash. We also make the constructors explicit in the concrete classes with no need to call a subperclass constructor. We did some basic benchmarks to compare performance with current master. On this branch GridQubit.__hash__ dropped from 1.3us to 488ns (2.66x speedup), while repeated hashing of the same grid qubit dropped from 150ns to 121ns (1.24x speedup). Similarly, LineQubit.__hash__ dropped from 685uns to 453ns (1.51x speedup), while repeated hashing of the same line qubit dropped from 142ns to 124ns (1.15x speedup).

On master:

In [2]: gq = cirq.GridQubit(1, 2)

In [3]: %timeit hash(gq)
150 ns ± 1.66 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [4]: %timeit hash(cirq.GridQubit(1, 2))
1.3 µs ± 25.2 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [5]: lq = cirq.LineQubit(3)

In [6]: %timeit hash(lq)
142 ns ± 2.85 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [7]: %timeit hash(cirq.LineQubit(3))
685 ns ± 14.1 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

and on this branch:

In [2]: gq = cirq.GridQubit(1, 2)

In [3]: %timeit hash(gq)
121 ns ± 0.737 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [4]: %timeit hash(cirq.GridQubit(1, 2))
488 ns ± 2.16 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [5]: lq = cirq.LineQubit(3)

In [6]: %timeit hash(lq)
124 ns ± 1.53 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [7]: %timeit hash(cirq.LineQubit(3))
453 ns ± 2.19 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

@maffoo maffoo requested review from vtomole, cduck and a team as code owners November 17, 2023 07:59
@maffoo maffoo requested a review from dabacon November 17, 2023 07:59
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3c81961) 97.84% compared to head (c43f689) 97.84%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6350   +/-   ##
=======================================
  Coverage   97.84%   97.84%           
=======================================
  Files        1110     1110           
  Lines       96656    96696   +40     
=======================================
+ Hits        94572    94612   +40     
  Misses       2084     2084           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maffoo
Copy link
Contributor Author

maffoo commented Nov 17, 2023

Updated to optimized NamedQubit as well; first-time hashing sped up from 1.78us to 553ns (3.2x speedup) and repeated hashing sped up from 220ns to 162ns (1.36x speedup).

On master:

In [2]: nq = cirq.NamedQubit("abc")

In [3]: %timeit hash(nq)
220 ns ± 14.1 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: %timeit hash(cirq.NamedQubit("abc"))
1.78 µs ± 13.9 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

On this branch:

In [2]: nq = cirq.NamedQubit("abc")

In [3]: %timeit hash(nq)
162 ns ± 4.14 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [4]: %timeit hash(cirq.NamedQubit("abc"))
553 ns ± 1.16 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

@maffoo maffoo changed the title Speed up GridQubit and LineQubit hashing Speed up hashing for GridQubit, LineQubit, and NamedQubit Nov 17, 2023

def __hash__(self) -> int:
if self._hash is None:
self._hash = hash((self._row, self._col, self._dimension))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just compute this in the constructor? It should be immutable, right? That would remove the need for an if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but since construction is more common than hashing I think it would be good to keep the constructor as fast as possible. I played around a bit with adding a __new__ method so we can actually cache instances instead of allocating new ones each time the constructor is called, but that interacts with pickling in strange ways that I haven't worked out yet.

Copy link
Contributor Author

@maffoo maffoo Nov 17, 2023

Choose a reason for hiding this comment

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

Here's a comparison of timings with lazy hashing (this PR) vs eager hashing where as you suggest the hash is computed in the constructor and there's no conditional in __hash__. It does indeed make hashing faster, but slows down the constructor quite a bit:

Operation Lazy Hash [ns] Eager Hash [ns] t_eager / t_lazy
%timeit cirq.GridQubit(1, 2) 294 430 1.46x
%timeit hash(cirq.GridQubit(1, 2)) 652 589 0.90x
q = cirq.GridQubit(1, 2); %timeit hash(q) 163 135 0.83x
%timeit cirq.LineQubit(3) 254 369 1.45x
%timeit hash(cirq.LineQubit(3) 560 506 0.90x
q = cirq.LineQubit(3); %timeit hash(q) 163 144 0.88x
%timeit cirq.NamedQubit("abc") 249 361 1.45x
%timeit hash(cirq.NamedQubit("abc") 568 502 0.88x
q = cirq.NamedQubit("abc"); %timeit hash(q) 159 137 0.86x

So eager hashing speeds up the __hash__ calls by between 10-20%, but slows down the constructors by almost 50%. If we can figure out __new__ to cache qid instances, then I think eager hashing would be a clear win, but for now I'd suggest we stick with lazy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks for the thorough analysis. This makes sense.

@maffoo maffoo requested a review from dstrain115 November 17, 2023 17:50
@maffoo maffoo merged commit 26dbabc into master Nov 17, 2023
38 checks passed
@maffoo maffoo deleted the u/maffoo/qid-hashes branch November 17, 2023 19:04
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants