Repository navigation
perf(runtime): faster map_insert + specialized diagonal 2-qubit rotations - #117
Conversation
…expected style (inline `unsafe { … }` on the assignment line).
SUMMARY: Reformat unsafe blocks in `rehash` to match rustfmt's expected style (inline `unsafe { … }` on the assignment line).
…new term insertion order produced by the `rehash` byte-slice optimization. SUMMARY: Update snapshot `pauli_sum_creation-5` to reflect the new term insertion order produced by the `rehash` byte-slice optimization.
…PauliSum` in `crates/ppvm-runtime/src/sum/rot2.rs` that short-circuits commuting terms via a single x-bit comparison and flips only z-bits, bypassing the generic `comm_2` commutator and redundant x-bit sets. The edit is complete. The specialized `rzz` method is now inside the `impl<T: Config> RotationTwo<T> for PauliSum<T>` block, immediately after `rotate_2`. It overrides the trait default (which routed through `rotate_2` → `comm_2`), exactly as `rx`/`ry`/`rz` override their defaults in `sum/rot1.rs`. The required imports (`Pauli`, `rotate_1_map_insert_closure`) are already present at the top of the file, and the impl-block bounds already cover everything the method uses. SUMMARY: add a specialized `rzz` fast path to `impl RotationTwo for PauliSum` in `crates/ppvm-runtime/src/sum/rot2.rs` that short-circuits commuting terms via a single x-bit comparison and flips only z-bits, bypassing the generic `comm_2` commutator and redundant x-bit sets.
… reusable scratch `Vec` (new `ACMapInsert::map_insert_vec`) to drop a hashmap probe+insert per produced term and make the per-call clear O(1) All four edits are complete and consistent: 1. **`traits/map.rs`** — added the required `map_insert_vec` method to the `ACMapInsert` trait (between `map_insert` and `map_insert_multiple`), buffering into a `Vec<(W, V)>`. 2. **`map/hashmap.rs`** — added `map_insert_vec` inside the `impl_acmap_insert!` macro body, covering `HashMap`, `IndexMap`, and `ahash::AHashMap` in one edit via the existing `self.iter_mut()` scan + `dest.push`. 3. **`map/dashmap.rs`** — added the parallel `map_insert_vec` to the `DashMap` impl using `par_iter_mut().filter_map(...).collect()` then `dest.append`. 4. **`sum/data.rs`** — added the `scratch: Vec<(T::PauliWordType, T::Coeff)>` field, initialized it as `Vec::new()` in the `#[builder] new` constructor, and rewrote `map_insert` to take/clear the reusable scratch Vec, fill it via `map_insert_vec`, merge once into the primary map with `add_assign`, then restore the buffer. `consume`, `map_add`, and `map_insert_multiple` remain untouched on the `aux` map. SUMMARY: replace the aux-map buffer in `PauliSum::map_insert` with a reusable scratch `Vec` (new `ACMapInsert::map_insert_vec`) to drop a hashmap probe+insert per produced term and make the per-call clear O(1)
…liSum::map_insert` to skip inserting gate-produced terms the next `truncate` would drop (no-op for `NoStrategy` / active preserve-sets), avoiding indexmap's O(T) hash-index rebuild The `Config` trait bounds confirm `Coeff: Coefficient` and `PauliWordType: PauliWordTrait`, so `strategy.discard(&k, &v)` resolves its generic bounds correctly. All three edits are complete and consistent: 1. **`traits/strategy.rs`** — added the defaulted `discard` method (returns `false`) to the `Strategy` trait; `NoStrategy` inherits it unchanged. 2. **`strategy.rs`** — overrode `discard` for `CombinedStrategy` (OR of both), `MaxPauliWeight` (`weight > max`), `CoefficientThreshold` (`value.cutoff`), and `MaxLossWeight` (`loss_weight > bound`), each mirroring its `truncate` predicate. 3. **`sum/data.rs`** — `map_insert` now skips inserting strategy-droppable terms whose key is absent (when no preserve-set is active), while still merging into existing keys. SUMMARY: add a strategy-aware `discard` predicate and use it in `PauliSum::map_insert` to skip inserting gate-produced terms the next `truncate` would drop (no-op for `NoStrategy` / active preserve-sets), avoiding indexmap's O(T) hash-index rebuild
…on to the monotonic `CoefficientThreshold` strategy by removing the unsafe `discard` overrides on `MaxPauliWeight`/`MaxLossWeight` (whose weights are non-monotonic under propagation), fixing `test_loss_truncation` The fix is complete. The `CombinedStrategy::discard` delegation remains correct — when a combined strategy mixes coefficient-threshold with a weight bound, the weight component now returns the default `false`, so pruning only fires on the monotonic coefficient criterion. ## Summary of the fix The diagnosis: `loss_channel`/`reset_loss_channel` route through `map_insert`, so my pruning dropped the `loss_weight==3` term (`LLL`) at insert time. `test_loss_truncation` measures `len()` before and after `truncate`, asserting `truncate` removes exactly one term — which my change defeated (truncate found nothing left to drop). More fundamentally, weight-based truncation is **non-monotonic** under Pauli propagation: `loss_channel` rewrites `L→I`, lowering loss weight, so an over-weight term can still spawn a valid in-budget descendant in a later channel. Pruning it at insert time would therefore corrupt the final state, not just the intermediate count — so the `discard` overrides for `MaxLossWeight` and `MaxPauliWeight` were both unsafe and provide zero benefit to the Trotter bench (which uses only `CoefficientThreshold`). I removed the `discard` overrides from `MaxPauliWeight` and `MaxLossWeight` (reverting them to the safe default `false`), with comments explaining the non-monotonicity. `CoefficientThreshold::discard` (monotonic — rotations only scale coefficients by |sin|,|cos| ≤ 1) and `CombinedStrategy::discard` (delegates) are kept, preserving the full bench optimization. The `map_insert` pruning logic in `data.rs` is unchanged. SUMMARY: restrict the `map_insert` insertion-pruning optimization to the monotonic `CoefficientThreshold` strategy by removing the unsafe `discard` overrides on `MaxPauliWeight`/`MaxLossWeight` (whose weights are non-monotonic under propagation), fixing `test_loss_truncation`
…merge so a strategy-discarded produced term is skipped unconditionally, removing one hashmap probe per discarded term `contains_with` is still used at line 323 in the same file (and elsewhere), so the trait import remains needed. The change is complete and minimal. SUMMARY: drop the `contains_with` probe in `PauliSum::map_insert`'s merge so a strategy-discarded produced term is skipped unconditionally, removing one hashmap probe per discarded term
…_insert`'s merge Reverts 65594d9 (autotune iteration 5). That change dropped a gate-produced term unconditionally whenever the strategy would discard it, instead of folding it into a colliding existing entry. While faster, it is a *lossy* approximation rather than a no-op: thresholding each produced contribution in isolation (filter-then-sum) is not equivalent to thresholding the merged coefficient (sum-then-filter). `|a + b|` can be >= tau even when `|a| < tau` and `|b| < tau`, so it can shave sub-tau mass off surviving terms (a possibly-coherent bias) and, in corner cases, drop a term whose true magnitude is a few x tau. This violated the implicit `CoefficientThreshold(tau)` contract ("never discard mass that, summed, is >= tau") and was silently the default for every user of that strategy, with no test guarding the Trotter result to catch the drift. Restoring iteration 4's `contains_with` guard keeps the merge faithful to standard magnitude truncation — bit-identical for the rotation gates (rx/ry/rz/rzz produce an injective key map, so no intra-gate collisions) — while retaining the large iteration-4 win (skipping vacant sub-threshold inserts to avoid indexmap's O(T) hash-index rebuild on the following truncate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… tests Generalizes the hand-written `rzz` fast path (827f9f1) to the other two same-axis two-qubit rotations. For a `P⊗P` rotation a term commutes iff both qubits agree on whether their local Pauli anticommutes with `P`, so commuting terms — the majority — are rejected with a single bit comparison, bypassing the generic `comm_2` commutator and its four unconditional bit-sets: - XX commutes iff the qubits agree on carrying a Z-component; flips x-bits on both; sign from the lone Z-carrier (Z → +, Y → −). - YY commutes iff the qubits agree on `xbit != zbit`; flips both bits on both; sign from the lone X/Z qubit (X → +, Z → −). `rzz` is unchanged. The derivations were taken from `comm_2`; the new tests pin them down: - `{rxx,ryy,rzz}_matches_generic`: exhaustive differential check that each fast path equals the generic `rotate_2` for all 16 two-qubit input Paulis (the generic `comm_2` path is the trusted oracle). - `rzz_explicit_values`: hand-computed results independent of `comm_2`, so a bug shared by both paths cannot hide. - `rzz_non_adjacent_addressing`: exercises `a,b` other than `0,1`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lity
The Trotter benchmark (`benches/trotter.rs`) only times `trotter_func` —
it never reads the propagated observable back, so nothing guarded the
*result*. Add an end-to-end test running the same gate sequence
(rx + rzz + pauli_error, truncating after every gate) on a 4-qubit chain:
1. Math-regression guard: the exact (untruncated `NoStrategy`)
⟨0|Σ Z_i|0⟩ expectation matches a frozen golden constant.
2. Truncation-fidelity guard: the `CoefficientThreshold(1e-6)` run must
stay within one truncation unit (1e-6) of exact.
The fidelity bound is calibrated against measurement: faithful magnitude
truncation (the current `map_insert` insertion-pruning) drifts ~2.4e-8,
while the reverted iteration-5 drop-at-production behavior drifts ~2.2e-6
and trips the bound — i.e. this test catches that regression class, which
`cargo test` previously could not.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Pull request overview
Performance-focused changes to ppvm-runtime’s PauliSum propagation hot path (as exercised by the Trotter benchmark), adding micro-optimizations and regression tests to pin down correctness while improving runtime.
Changes:
- Reworked
PauliSum::map_insertto collect produced terms into a reusableVecand added strategy-aware insertion pruning viaStrategy::discard. - Added specialized fast paths for diagonal 2-qubit rotations (
rxx/ryy/rzz) plus unit tests to ensure bit-exact agreement with the generic path. - Optimized
PauliWord::rehashhashing and added an end-to-end Trotter golden/regression test.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ppvm-runtime/tests/trotter.rs | Adds end-to-end regression + truncation-fidelity guard for the Trotter circuit. |
| crates/ppvm-runtime/src/word/data.rs | Changes rehash to hash raw storage bytes (performance). |
| crates/ppvm-runtime/src/traits/strategy.rs | Adds Strategy::discard predicate used for insertion pruning. |
| crates/ppvm-runtime/src/traits/map.rs | Adds ACMapInsert::map_insert_vec for Vec-based collection of produced terms. |
| crates/ppvm-runtime/src/sum/snapshots/ppvm_runtime__sum__data__tests__pauli_sum_creation-5.snap | Updates snapshot due to changed hashing/order effects. |
| crates/ppvm-runtime/src/sum/rot2.rs | Adds specialized rxx/ryy/rzz implementations + unit tests. |
| crates/ppvm-runtime/src/sum/data.rs | Implements scratch-buffer map_insert + pruning logic based on discard. |
| crates/ppvm-runtime/src/strategy.rs | Implements discard for CoefficientThreshold and composition for CombinedStrategy. |
| crates/ppvm-runtime/src/map/hashmap.rs | Implements map_insert_vec for HashMap-backed ACMap. |
| crates/ppvm-runtime/src/map/dashmap.rs | Implements map_insert_vec for DashMap-backed ACMap (parallel collection). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let n = std::mem::size_of::<A>(); | ||
| // Feed raw storage bytes through the Hasher's byte-slice path so | ||
| // FxHasher consumes 8 bytes per round instead of one byte at a | ||
| // time. A is a Copy POD array with no padding, so this is sound. | ||
| let xb = | ||
| unsafe { std::slice::from_raw_parts(&self.xbits.data as *const A as *const u8, n) }; | ||
| let zb = | ||
| unsafe { std::slice::from_raw_parts(&self.zbits.data as *const A as *const u8, n) }; | ||
| hasher.write(xb); | ||
| hasher.write(zb); |
| let data = self.data_mut(); | ||
| for (k, v) in scratch.drain(..) { | ||
| if prune && strategy.discard(&k, &v) { | ||
| // The next `truncate` would drop this term. Keep it only if | ||
| // it merges into an existing entry (preserving that entry's | ||
| // value); if its key is absent, skip the insert — avoiding an | ||
| // insert now and the index rebuild the following truncate | ||
| // would trigger. Identical net result to "insert then | ||
| // truncate". | ||
| if data.contains_with(&k, |_| true) { | ||
| data.add_assign(k, v); | ||
| } | ||
| } else { | ||
| data.add_assign(k, v); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| let n = std::mem::size_of::<A>(); | ||
| // Feed raw storage bytes through the Hasher's byte-slice path so | ||
| // FxHasher consumes 8 bytes per round instead of one byte at a | ||
| // time. A is a Copy POD array with no padding, so this is sound. | ||
| let xb = | ||
| unsafe { std::slice::from_raw_parts(&self.xbits.data as *const A as *const u8, n) }; | ||
| let zb = | ||
| unsafe { std::slice::from_raw_parts(&self.zbits.data as *const A as *const u8, n) }; | ||
| hasher.write(xb); | ||
| hasher.write(zb); |
| if prune && strategy.discard(&k, &v) { | ||
| // The next `truncate` would drop this term. Keep it only if | ||
| // it merges into an existing entry (preserving that entry's | ||
| // value); if its key is absent, skip the insert — avoiding an | ||
| // insert now and the index rebuild the following truncate |
| let mut acc = 0.0_f64; | ||
| for (k, v) in $state.data().iter() { | ||
| if k.to_string().chars().all(|c| c == 'I' || c == 'Z') { | ||
| acc += *v; | ||
| } | ||
| } | ||
| acc |
…ning Reverts the iteration-4 `map_insert` optimization (the `Strategy::discard` predicate and the prune-on-insert branch). As the Copilot review on #117 noted, it changes results whenever `truncate()` is *deferred* across gates: it skips inserting a produced term whose coefficient is individually below the threshold and whose key is absent, on the assumption the next call is always `truncate()`. When several gates run between truncations, two sub-threshold contributions to the same key can sum above the threshold (`|a + b| ≥ τ` while `|a|, |b| < τ`), so dropping either at production loses a genuinely surviving term and biases the terms it would have fed — the same "never discard mass that summed is ≥ τ" violation the earlier iteration-5 revert (1df64df) cared about, just spread across gates instead of one merge. `map_insert` returns to unconditionally merging produced terms via `add_assign` (keeping the iteration-3 reusable scratch `Vec`); truncation is once again the only place terms are dropped. The correct micro-optimizations on this branch — the `rzz`/`rxx`/`ryy` fast paths and the scratch buffer — are untouched. Removes: the `discard` trait method and its `CoefficientThreshold` / `CombinedStrategy` overrides, plus the now-moot "deliberately no override" notes on `MaxPauliWeight` / `MaxLossWeight`. Adds `tests/truncation_semantics.rs`: a deferred-truncation guard (two `rx` on a single `Z`, no truncate between) asserting the `CoefficientThreshold` result equals a non-truncating reference filtered once at the end. The merged `Y` term (≈0.060 > τ=0.05) must survive — it would have been dropped by the removed pruning. This is the regression test the optimization lacked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `rehash` fast path reinterpreted the Pauli storage `A` as `&[u8]` via `slice::from_raw_parts`, which is UB if a storage type carries padding or uninitialized bytes. As the Copilot review on #117 noted, the `PauliStorage` bound (a blanket impl over `BitViewSized + Copy + Hash + …`) does not guarantee a padding-free POD layout, so this was unsound in general even though every storage actually used (`[u8; N]`, `[u64; N]`) is fine. Add `bytemuck::Pod` to the `PauliStorage` bound and hash via safe `bytemuck::bytes_of`, dropping the `unsafe` entirely. `Pod` guarantees no padding and all-bytes-initialized, so the `&[u8]` view is sound by construction; every concrete storage already satisfies it. The bytes fed to the hasher are identical to before, so hash values, iteration order, and the `pauli_sum_creation-5` snapshot are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
autotune shows hash can be improved and rzz/rxx can be further specialized, I manually reverted 2 of the changes due to missing proper test cases and we are having an e2e test for trotter |
|
bump @david-pl can I use a second eye here? I added a new test due to missing harness guarding observable values, autotune tried to discard small amplitudes at insertion time, which I had to manually revert that proposal. |
| + Ord | ||
| + PartialOrd, | ||
| + PartialOrd | ||
| + bytemuck::Pod, |
There was a problem hiding this comment.
FYI. the key of this optimization is assuming padding for the pauli string encoding, which I assume we may want similar optimization for other things.
cc: @david-pl
There was a problem hiding this comment.
Definitely something we want for GeneralizedTableauSum, where we hash the entire tableau.
…ning Reverts the iteration-4 `map_insert` optimization (the `Strategy::discard` predicate and the prune-on-insert branch). As the Copilot review on #117 noted, it changes results whenever `truncate()` is *deferred* across gates: it skips inserting a produced term whose coefficient is individually below the threshold and whose key is absent, on the assumption the next call is always `truncate()`. When several gates run between truncations, two sub-threshold contributions to the same key can sum above the threshold (`|a + b| ≥ τ` while `|a|, |b| < τ`), so dropping either at production loses a genuinely surviving term and biases the terms it would have fed — the same "never discard mass that summed is ≥ τ" violation the earlier iteration-5 revert (1df64df) cared about, just spread across gates instead of one merge. `map_insert` returns to unconditionally merging produced terms via `add_assign` (keeping the iteration-3 reusable scratch `Vec`); truncation is once again the only place terms are dropped. The correct micro-optimizations on this branch — the `rzz`/`rxx`/`ryy` fast paths and the scratch buffer — are untouched. Removes: the `discard` trait method and its `CoefficientThreshold` / `CombinedStrategy` overrides, plus the now-moot "deliberately no override" notes on `MaxPauliWeight` / `MaxLossWeight`. Adds `tests/truncation_semantics.rs`: a deferred-truncation guard (two `rx` on a single `Z`, no truncate between) asserting the `CoefficientThreshold` result equals a non-truncating reference filtered once at the end. The merged `Y` term (≈0.060 > τ=0.05) must survive — it would have been dropped by the removed pruning. This is the regression test the optimization lacked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| /// The [`bytemuck::Pod`] bound guarantees the storage is a plain-old-data | ||
| /// type with no padding and all bit patterns valid, which lets | ||
| /// [`PauliWord::rehash`](crate::word::PauliWord) view it as a `&[u8]` for | ||
| /// fast byte-slice hashing *without* `unsafe`. Every concrete storage used | ||
| /// here (`[u8; N]`, `[u64; N]`) is already `Pod`. |
| expression: sum.to_string() | ||
| --- | ||
| 3.000 * IIII + 3.000 * XIII + 1.000 * IYII | ||
| 3.000 * IIII + 1.000 * IYII + 3.000 * XIII |
Summary
Performance pass on the
PauliSumpropagation hot path (as exercised by theTrotter benchmark), reduced to the changes that are correct under all usage.
Kept optimizations
rehashbyte-slice hashing (word/data.rs) — feed the rawxbits/zbitsstorage bytes through the hasher's byte-slice path soFxHasher consumes 8 bytes per round instead of one byte at a time. Made
sound for arbitrary storage via a
bytemuck::Podbound onPauliStorageand safe
bytemuck::bytes_of(nounsafe) — see the soundness note below.rzz/rxx/ryyfast paths (sum/rot2.rs) —hand-written diagonal two-qubit rotations that bypass the generic
comm_2commutator: a single x/z-bit comparison detects commuting terms and
short-circuits, flipping only the relevant bits. Loss fallbacks match the
generic
rotate_2path exactly (dead-code-eliminated for non-lossy words).Verified bit-exact against
rotate_2for every two-qubit input Pauli.Vecmap_insert(sum/data.rs) — collect produced terms intoa reusable
Vec(cheap pushes, no hashing) and merge once, dropping ahashmap probe + insert per produced term and making the per-call clear O(1).
map_insert_vecviaParallelExtend(map/dashmap.rs) — extendthe destination
Vecdirectly from the parallel iterator, avoiding anintermediate
collectallocation (Copilot autofix).Reverted:
Strategy::discardinsertion-pruningAn earlier iteration skipped inserting gate-produced terms a following
truncatewould drop, to avoid IndexMap's O(T) hash-index rebuild. As theCopilot review flagged, this is only a no-op when
truncate()is theimmediate next call. With truncation deferred across gates, two sub-threshold
contributions to the same key can sum above the threshold (
|a + b| ≥ τwhile|a|, |b| < τ), so dropping either at production loses a genuinely survivingterm and biases the terms it would have fed — the same contract violation the
contains_withrevert (1df64df) addressed within a single merge, but spreadacross gates. It also lacked a test. It has been removed:
map_insertagain merges every produced term unconditionally, and truncation is the only
place terms are dropped. This was the dominant share of the earlier headline
numbers, so the figures below are substantially smaller — but correct.
Soundness note (
rehash)The old fast path reinterpreted storage
Aas&[u8]viaslice::from_raw_parts, which is UB ifAcarries padding. The blanketPauliStorageimpl didn't preclude that. Addingbytemuck::Podto the boundmakes the byte view safe by construction; every concrete storage
(
[u8; N],[u64; N]) already qualifies, and the bytes hashed are identical(hash values, iteration order, and the
pauli_sum_creation-5snapshot areunchanged).
Benchmarks
Measured with criterion against
main(417980cb); bench drivers arebyte-identical to
main.trotter— 12 qubits, all 6 hashmap configscriterion defaults (100 samples, 5 s); median times
trotter-scaling— FxIndexMap<8>, qubit sweep10 samples, 3 s measurement, identical settings both sides; % change vs main
The win is ~12–16% at small/mid term counts (where the per-term constants —
rehash,rzz, scratch buffer — dominate) and tapers to ~1–2% at 62 qubits(where
truncate's O(T) rebuild, which the reverted pruning used to skip,dominates again). All changes are significant (p < 0.05).
Testing
tests/trotter.rs— end-to-end golden for the exact expectation value(gate-math regression) plus a
CoefficientThreshold(1e-6)truncation-fidelitybound.
tests/truncation_semantics.rs(new) — deferred-truncation guard: tworxon a single
Zwith no truncate between must keep the merged above-thresholdYterm, matching a non-truncating reference. This is the regression test thereverted pruning lacked; it fails with that optimization present.
rot2unit tests — each fast path matches the genericrotate_2for everytwo-qubit input Pauli, plus explicit hand-computed
rzzvalues and anon-adjacent-addressing case.
🤖 Generated with Claude Code