perf(tableau-sum): 5.04x faster GeneralizedTableauSum build (lazy branch materialization + faster fingerprinting)#157
Open
david-pl wants to merge 27 commits into
Open
perf(tableau-sum): 5.04x faster GeneralizedTableauSum build (lazy branch materialization + faster fingerprinting)#157david-pl wants to merge 27 commits into
david-pl wants to merge 27 commits into
Conversation
loss_channel and single-qubit pauli_error now describe each branch as a BranchMutation of a parent entry instead of deep-cloning a tableau up front. The merge resolves structural identity against the virtual (parent + mutation) tableau via structurally_equal_mutated and only clones the parent when the branch survives as a new entry; merges and below-cutoff drops never clone. The VecStorage path is the optimized one; MapStorage materializes parents eagerly (correctness only). Math is unchanged: msd-noisy benchmark still ends at exactly 2025 branches with sum_p = 1.0, and all 76 ppvm-tableau-sum tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ltas Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…xhash) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gather+gxhash)" This reverts commit 34b41ce.
…compare Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-iteration log, metric ledger, and prompt records were working notes for the tuning session; they are summarized in the PR description rather than checked in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Pull request overview
This PR speeds up ppvm-tableau-sum’s GeneralizedTableauSum build phase by avoiding deep clones during single-qubit noise branching and by reducing fingerprinting overhead, while aiming to keep sampling behavior unchanged.
Changes:
- Add lazy branch materialization via
BranchMutation+ storage support for merging “virtual” branches without cloning unless they survive as new entries. - Speed up fingerprint maintenance by bulk-hashing row word bytes and precomputing per-row/qubit phase/loss masks (
RowMasks), plus faster per-column bit reads in noise hot loops. - Add a deterministic
msd-noisytiming/accuracy harness example for reproducible benchmarking.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
crates/ppvm-tableau-sum/src/storage/vec.rs |
Implements lazy mutated-branch merging for VecStorage and reduces per-entry hash rebuild overhead with shared RowMasks. |
crates/ppvm-tableau-sum/src/storage/mod.rs |
Adds BranchMutation, lazy structural equality checks, bulk word hashing, RowMasks, and fast bit reads used by noise and storage merging. |
crates/ppvm-tableau-sum/src/storage/map.rs |
Adds a correctness-first fallback implementation of mutated-branch insertion for MapStorage and uses shared RowMasks when iterating. |
crates/ppvm-tableau-sum/src/storage/entry_store.rs |
Extends EntryStore with an API for inserting/merging lazily-described mutated branches. |
crates/ppvm-tableau-sum/src/noise.rs |
Switches loss_channel/pauli_error to emit virtual branches (parent index + mutation) and compute phase/loss deltas without cloning. |
crates/ppvm-tableau-sum/examples/msd-noisy-bench.rs |
Adds a deterministic performance harness intended to guard against math-changing regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+51
to
+60
| /// View a `Copy` plain-old-data value's bytes. Sound because `A: PauliStorage` | ||
| /// implies `bytemuck::Pod`: no padding, every bit pattern valid, so the bytes | ||
| /// are fully initialized and `u8`-aligned. | ||
| #[inline] | ||
| fn pod_bytes<A: Copy>(value: &A) -> &[u8] { | ||
| // SAFETY: `A` is POD (PauliStorage: bytemuck::Pod); reading its | ||
| // `size_of::<A>()` initialized bytes as `[u8]` is sound, and the borrow is | ||
| // tied to `value`. | ||
| unsafe { std::slice::from_raw_parts(value as *const A as *const u8, std::mem::size_of::<A>()) } | ||
| } |
Comment on lines
+318
to
+323
| BranchMutation::Pauli { op, addr0 } => match op { | ||
| 1 => tab.x(addr0), | ||
| 2 => tab.y(addr0), | ||
| 3 => tab.z(addr0), | ||
| _ => {} | ||
| }, |
Comment on lines
+233
to
+241
| // Accuracy guard: the optimizations under test must not change the math, | ||
| // so the final branch count must stay at the baseline value. | ||
| const EXPECTED_BRANCHES: usize = 2025; | ||
| if branches != EXPECTED_BRANCHES { | ||
| eprintln!( | ||
| "WARNING: branch count {} != baseline {} — accuracy/structure changed!", | ||
| branches, EXPECTED_BRANCHES | ||
| ); | ||
| } |
Comment on lines
+12
to
+16
| use ppvm_pauli_sum::config::fx64hash::Byte8F64; | ||
| use ppvm_tableau::prelude::*; | ||
| use ppvm_tableau_sum::data::GeneralizedTableauSum; | ||
| use ppvm_tableau_sum::storage::EntryStore; | ||
|
|
Replace the hand-rolled `unsafe pod_bytes` byte view with `bytemuck::bytes_of`. `PauliStorage` already requires `bytemuck::Pod`, so the byte view is sound without `unsafe`, matching the existing idiom in `PauliWord::rehash`. Identical codegen (same pointer cast), so build time and accuracy are unchanged (branches=2025, sum_p2 bit-identical). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
david-pl
force-pushed
the
autotune/tableau-sum-build
branch
from
2319212 to
e35ddd9
Compare
Collaborator
|
@david-pl I want to merge this pr so I can start refactor the trait system |
The depolarizing-branch op was a `u8` (1=X, 2=Y, 3=Z), so both matches on it carried a dead `_` catch-all that silently ignored invalid ops. Reuse the existing `ppvm_pauli_word::pattern::NotIdentity` enum (X/Y/Z) instead, which makes `apply_branch_mutation` and the `structurally_equal_mutated` flip rule exhaustive with no catch-all — the invalid state is now unrepresentable. Promotes `NotIdentity` from `pub(crate)` to `pub` and re-exports it from the `pattern` module. Matching is by variant name, so the enum's `X=1, Z=2, Y=3` discriminants don't affect behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@Roger-luo I went through it again and I think it's in an okay shape. So, fine to merge since you want to run cleanup anyway. |
Collaborator
|
Let's merge this. I want to do a small refactor on the tree system afterwards anyways. It's going to be easier to think about new abstractions after merging. |
Comment on lines
59
to
62
| where | ||
| T: Config, | ||
| I:, | ||
| C: SparseVector<Complex<T::Coeff>, I>, |
Comment on lines
+262
to
+264
| // Surviving new entry: materialize now (clone parent + | ||
| // apply mutation). Later branches' parent_idx still refer | ||
| // to the original entries — push never moves them. |
Comment on lines
+68
to
+72
| fn insert_or_merge_mutated_branches( | ||
| &mut self, | ||
| branches: Vec<(usize, BranchMutation, T::Coeff, u64, u64)>, | ||
| cutoff: &T::Coeff, | ||
| ) -> bool; |
Comment on lines
+233
to
+241
| // Accuracy guard: the optimizations under test must not change the math, | ||
| // so the final branch count must stay at the baseline value. | ||
| const EXPECTED_BRANCHES: usize = 2025; | ||
| if branches != EXPECTED_BRANCHES { | ||
| eprintln!( | ||
| "WARNING: branch count {} != baseline {} — accuracy/structure changed!", | ||
| branches, EXPECTED_BRANCHES | ||
| ); | ||
| } |
Comment on lines
+329
to
+333
| pub(crate) fn structurally_equal_mutated<T, I, C>( | ||
| existing: &GeneralizedTableau<T, I, C>, | ||
| parent: &GeneralizedTableau<T, I, C>, | ||
| m: BranchMutation, | ||
| scratch: &mut FxHashMap<I, Complex<T::Coeff>>, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Autotune session optimizing
GeneralizedTableauSumbuild time on themsd-noisyworkload (magic-state-distillation circuit with single-qubit depolarizing + loss noise).Build time: 2620 ms → 520 ms — a 5.04× speedup. Sampling throughput is unchanged (~22 µs/shot) and the result is bit-identical: branch count stays
2025,sum_p2 = 0.725135705447, and the top-5 probabilities match to the recorded reference. All changes live incrates/ppvm-tableau-sum/src/.What landed
gxhash64, instead of ~340 tiny hash writesbitvecper-bit indexing with direct storage-word access in the noise hot loopsThe dominant lever was #1: profiling showed
fork/clone was 47% of build (32% rawmemmove); after lazy materialization it dropped to ~0.1%.Detail on #1 (lazy materialization)
GeneralizedTableauX/Y/Z gates only flip per-row sign bits and leave words/coefficients/is_lostidentical to the parent; loss only sets oneis_lostbit. So a branch's fingerprint is derivable from the parent without cloning. This PR addsBranchMutation+structurally_equal_mutated+apply_branch_mutationinstorage/mod.rs, and aninsert_or_merge_mutated_branchestrait method (VecStoragedoes the lazy/index-based path;MapStoragekeeps an eager-clone fallback).loss_channelandpauli_erroremit virtual branches and only clone the survivors. The two-qubit / correlated-loss / reset-loss paths are left eager (not exercised bymsd-noisy).One discarded experiment (kept for the record)
A scalar single-pass word hash (replacing the gather +
gxhash) regressed to 921 ms and was reverted. This confirms the fingerprint rebuild is SIMD-throughput-bound — full re-hashing viagxhashis near-optimal, so the only remaining lever is to avoid re-hashing entirely (incremental/Zobrist fingerprinting). That is not in this PR.Where the time goes now (520 ms plateau)
phase_loss_hash14.5%) — proven SIMD-compute-boundThe next lever would be cell-level incremental ("Zobrist") fingerprinting maintained through the Clifford gates to skip the rebuild (~10–20% est.). It is deliberately not included — it's a large, clever change (new hash scheme + per-gate delta logic incl. cz cross-terms) that runs against the repo's "readability over cleverness" preference, for a modest further gain.
Testing
cargo test -p ppvm-tableau-sum— all crate tests passtest_word_fingerprint_cache_stays_consistentplus thebranches == 2025invariant in the bench harness, so any fingerprint drift fails loudly.Notes for reviewers
crates/ppvm-tableau-sum/examples/msd-noisy-bench.rsis the deterministic, seeded measurement harness used throughout (median-of-5 build time + accuracy fingerprint). It's included so the numbers are reproducible; drop it if you'd rather not carry a bench example.🤖 Generated with Claude Code