Uh oh!
There was an error while loading. Please reload this page.
Fuse MaxSim reduction at panel boundaries - #1365
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
juchen-ms (partychen)
commented
Sep 1, 2026
AI: blocking: The benchmark section identifies the compared commits, shapes, measurement count, and ordering, but not the host hardware. For these ISA-sensitive results, please also record the CPU model, core topology, OS, and relevant frequency/power configuration so the measurements are reproducible and comparable ( |
| b_row_bytes: NonZeroUsize, | ||
| b_rows: usize, | ||
| acc_bytes: usize, | ||
| work_bytes_per_b_row: usize, |
There was a problem hiding this comment.
AI: consider: work_bytes_per_b_row sounds like the complete working-set cost for one B row, but Plan::new multiplies it by AR. That makes the actual unit “bytes per A-row per B-row”; a future operation supplying an already-total per-B-row cost would over-reserve L1 by AR. Could we encode that unit in the parameter name (for example, work_bytes_per_a_row_per_b_row) and update the test wording? (names-match-vocabulary)
| for chain in &chains[1..] { | ||
| for (m, c) in merged.iter_mut().zip(chain) { | ||
| *m = m.max_simd(*c); | ||
| // `run` seeds state with a finite identity, and every update preserves that invariant. |
There was a problem hiding this comment.
AI: nit: The invariant here is “non-NaN,” not “finite”: valid inputs or overflow can produce +∞, which remains a valid running maximum. Could this say that run seeds a non-NaN identity and that placing the prior non-NaN state on the right preserves it when a candidate is NaN?
Context
This draft builds directly on #1333 at
4683fd8732988cbc9833186d55f1530b4c3b53b0and targetsusers/suryangupta/multi_vector_kernel_new_design. It preserves that PR's separation of layout, tiling, and leaf micro-kernels while exploring a narrower reduction boundary for MaxSim.Mark observed that materializing every raw A-panel x B-panel product into
Stripand draining it later adds avoidable traffic even though the leaf already knows when a complete panel result is available. This follow-up keeps the parent architecture and refines only that handoff.Problem
The current
Scratch/Slots/Accumulate/Draincontract writes tile-sized raw products and then makes a second pass to reduce them. Besides the extra memory traffic, the contract spreads MaxSim-specific behavior across non-local plumbing, and Scalar and V3 duplicate substantial storage/reduction machinery.Design
PanelOpinvoked after each full A-panel x B-panel contraction.kernels/strip.rsand the tile-sized intermediate allocation.This is an architecture/performance remediation of the parent refactor, not a new kernel implementation.
Correctness
The panel operation receives the same ordinal A/B panel positions as the previous drive contract, preserving full panels, B tails, A/B tile ordinal behavior, empty dimensions/documents, and padded query rows.
The reduction retains Rust
f32::maxbehavior around NaNs. In V3, finite running state is intentionally the right operand to the hardware max instruction; this preserves a valid prior maximum when one document lane produces NaN without introducing a generic standard-max comparison. A regression test covers this case.Performance methodology
The primary evidence is the original strict three-way comparison of:
3218478b5f7d1721840c29309e22d9fd74b1b4bc4683fd8732988cbc9833186d55f1530b4c3b53b0It used the official nine shapes, release builds, 50 measurements per shape, minimum ns/IP, and rotated implementation order to reduce ordering bias. Lower percentages are better. Reference is excluded from conclusions because this patch does not alter it. Auto duplicates V3 on this machine and is omitted below.
Aggregate results
The f16 V3 P90 for HEAD is dominated by one noisy/sensitive shape; the complete rows are included rather than hiding that variance.
Complete per-shape results
Scope / non-goals
This PR intentionally does not add V4 or Neon kernels, change MinMax or Spherical behavior, modify
ScopedAllocator, add cache detection, or introduce any other feature. It changes only the six MaxSim kernel paths needed to move reduction to the completed-panel boundary.Validation
cargo fmt --all --checkcargo test -p diskann-quantization multi_vector(82 passed)cargo clippy -p diskann-quantization --all-targets -- -D warningsLimitations
This machine does not support AVX-512, so no V4 execution claim is made. Standalone later runs, including the exact-final completion run, experienced machine-frequency drift; they are used only to confirm that the exact code completes every benchmark case, not as cross-version percentage evidence. The strict rotated three-way table above remains the primary performance comparison.