-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Preserve cached values in Circuit more granularly #7649
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
base: main
Are you sure you want to change the base?
Conversation
Okay, I lean toward not making the change in At some point, it would be possible to add a parameter to allow each use case to choose whether to create the cache or not, but that would be a separate PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7649 +/- ##
=======================================
Coverage 99.37% 99.37%
=======================================
Files 1078 1078
Lines 96162 96230 +68
=======================================
+ Hits 95559 95627 +68
Misses 603 603 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ts, update tests accordingly.
Updated PR to undo the change to |
This PR improves the resilience of the cached member fields in Circuit. Most importantly,
_placement_cache
, which is used to speed upCircuit.append()
from O(N) to O(1), is made to be resilient and consistent acrossinsert
and batch inserts,freeze/unfreeze cycles,add
andmul
operations, andcopy
. Prior to this PR, performing any of those actions on a circuit would delete the cache, andappend
would fall back to O(N) behavior from that point on.Summary of changes:
_PlacementCache
has new methodsinsert_moment(index, count)
andput(index, mop)
, that update the key indices to account for new moments or operations.Circuit
has new corresponding_insert_moments(...)
and_put_ops(...)
methods to modify the circuit and update the cache atomically, and logic strewn aroundCircuit
has been updated to use those methods when possible.Circuit.copy()
now preserves all cache values._placement_cache
are now immutable types, socopy
doesn't have to make copies.Moment.with_operations(*ops)
gave a 15% speedup in the common case oflen(ops)==1
.Overall, the performance of
append
andinsert
individually are unaffected by this change; measured time is equal, but the performance when mixing inserts and appends is improved from O(N) to O(1). It also cleans up the code a bit IMO, with less need to placeself._mutated()
everywhere.The biggest open question I have is whether building the placement cache inUpdate: it does not, and that functionality has been reverted.Circuit.from_moments()
justifies the extra latency. If not, that could be removed, and it would only affect the freeze/unfreeze cycle case.Fixes #7464