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

Improve GridQubit dictionary lookup performance 3x #3375

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Conversation

Strilanc
Copy link
Contributor

According to the following profiling code, this change decreases the dictionary lookup time from 2100ns to 600ns on my machine.

import time
import cirq

d = {q: 5 for q in cirq.GridQubit.rect(3, 3)}
q = cirq.GridQubit(0, 0)
total = 0
t0 = time.monotonic()
n = 100000
for _ in range(n):
    total += d[q]
t1 = time.monotonic()
print(total)
print((t1 - t0) / n * 10**9, "nanoseconds per hash")

According to the following profiling code, this change decreases the dictionary lookup time from 2100ns to 600ns on my machine.

```
import time
import cirq

d = {q: 5 for q in cirq.GridQubit.rect(3, 3)}
q = cirq.GridQubit(0, 0)
total = 0
t0 = time.monotonic()
n = 100000
for _ in range(n):
    total += d[q]
t1 = time.monotonic()
print(total)
print((t1 - t0) / n * 10**9, "nanoseconds per hash")
```
@Strilanc Strilanc requested a review from zchen088 September 30, 2020 23:05
@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Sep 30, 2020
Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

LGTM

# Explicitly implemented for performance (vs delegating to Qid).
if isinstance(other, GridQubit):
return self.row == other.row and self.col == other.col
return NotImplemented
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: most similar places I see around cirq invert the condition and return early instead, but up to you.

if not isinstance(other, GridQubit):
    return NotImplemented
return ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged.

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 30, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Sep 30, 2020
@CirqBot CirqBot merged commit 8d5a3af into master Sep 30, 2020
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 30, 2020
@CirqBot CirqBot deleted the faster_grid_qubit branch September 30, 2020 23:16
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants