Repository navigation
Support zero-column mutable matrix iterators - #1462
Conversation
Preserve logical rows and windows for N x 0 matrices during parallel mutable iteration instead of panicking.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (3)
Layout::new_unchecked(nrows, 0)is a new unsafe construction path for a zero-column layout. Since… · New The doc comment previously documented a panic for non-empty zero-column matrices; that behavior has… · Newrow(&self) -> &'a mut [T]is a safe API that can be called multiple times on the same&selfto… · New
What changed in this PR
This PR updates parallel mutable iterators for row-major matrices to correctly support N x 0 layouts by yielding empty mutable rows and zero-column windows instead of panicking.
Changes:
- Add an internal
ZeroColumnMuthelper to manufacture empty mutable row/window views for zero-column matrices (Rayon-only). - Update
MatrixMut::{par_rows_mut, par_window_iter_mut}to branch onncols == 0and return appropriate parallel iterators viaEither. - Update and extend Rayon tests to cover the non-empty zero-column case and validate coexistence of the produced views.
| File | Description |
|---|---|
| diskann-utils/src/views/rowmajor/iter.rs | Introduces ZeroColumnMut for constructing empty mutable views for zero-column matrices under Rayon. |
| diskann-utils/src/views/rowmajor.rs | Switches parallel iterator implementations to handle ncols == 0 via Either and adds/updates regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation preserves matrix shape, maintains zero-element aliasing invariants, and covers the new behavior with targeted tests.
Review effort: Balanced
Findings: None
Resolved since last review (3)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1462 +/- ##
==========================================
+ Coverage 91.90% 91.98% +0.07%
==========================================
Files 582 586 +4
Lines 114947 118245 +3298
==========================================
+ Hits 105640 108764 +3124
- Misses 9307 9481 +174
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Compare the original chunk-based par_rows_mut implementation with the current Either-backed implementation for 5x100 and 1,000,000x100 matrices.
Run with:
cargo bench -p diskann-utils --bench bench_main
Ten-run stability summary (Criterion midpoint estimates):
Implementation Median MAD CV Range
5x100 old 100.695 us 1.520 us 15.57% 54.311-104.710 us
5x100 current 100.595 us 1.715 us 3.49% 94.418-105.270 us
1Mx100 old 7.988 ms 0.088 ms 1.98% 7.680-8.189 ms
1Mx100 current 7.929 ms 0.078 ms 1.43% 7.747-8.108 ms
The old 5x100 CV is inflated by one anomalous 54.311 us run. The ratio of medians is -0.10% for 5x100 and -0.74% for 1Mx100, showing no measurable regression.
Restore the side-by-side comparison removed by 055e0a5 while naming the original implementation baseline and retaining current for the production implementation.
Use index-based row and window ranges with a shared ParMut helper instead of branching between zero-column views and Rayon slice chunks. Keep disjoint-view safety contracts beside the implementation and cover zero-column and nonempty coexistence under Miri.
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
Thanks! I had one quick question about whether or not the inclusion of criterion is worth it from a maintenance perspective. Otherwise, looks good to me!
# DiskANN v0.61.0 ## Highlights - **Quantization:** Added 3-bit spherical quantization with AVX2 acceleration, faster spherical encoding, and NEON float32 MinMax kernels. - **In-memory indexing (experimental):** Added product quantization and 1-8-bit scalar quantization, unified quantized representations, and AArch64 prefetching. - **Multi-vector search:** Added `i8 x i8` and MinMax8 x MinMax4 MaxSim kernels. - **Fixes:** Preserve virtual start-point out-edges when writing disk indexes, correctly round-trip scalar quantization configurations without standard deviation, and support zero-column mutable matrix iterators. ## Breaking changes and upgrading Changes are concentrated in quantization APIs and experimental/tooling crates. * PQ training (#1467) - Training now returns `BasicTable`. `SimplePivots` was removed. Replace `flatten()` with `view_pivots().as_slice()` and use `pivots_for(chunk)` for per-chunk access. * PQ distance tables (#1458) - Change `ProcessInto` bounds to `ProcessInto<f32>` and calls from `process_into::<Metric>(...)` to `process_into::<Metric, f32>(...)`. * MaxSim kernels (#1429) - Generic kernel/erasure code now requires `T: MaxSimElement` and score buffers of `T::Score` (`f32` for floats, `i32` for `i8`). Update `build_max_sim` error handling from `NotSupported` to `BuildMaxSimError`. * `diskann-inmem` (#1485) - Replace `repr::Spherical` with `repr::Quantized`. Wrap the quantizer in `repr::quantized::spherical::Spherical::new(...)` when configuring it. Configuration and `Rerank` types now live under `repr::quantized`. * `diskann-benchmark-runner` (#1475) - `input::Registered` and public `Registry::input` were removed. Use the concrete `Input` implementation's `tag()` and `example()` methods instead. ## All Changes * Fix serialization round trip for QuantizationType::SQ without standard deviation by @arrayka in #1461 * i8 x i8 MaxSim Kernels by @suri-kumkaran in #1429 * Adding `subview_mut` to `MatrixMut` by @arkrishn94 in #1463 * Add InsertResult::SuccessUpdate by @metajack in #1449 * [diskann-quantization] PQ Infrastructure for `diskann-inmem` by @hildebrandmw in #1458 * [diskann-quantization] Teach PQ training to return a `BasicTable`. by @hildebrandmw in #1467 * [deps] Run `cargo-update` by @hildebrandmw in #1452 * Support zero-column mutable matrix iterators by @arrayka in #1462 * [diskann-quantization] Use `f32::NAN` instead of `core::f32::NAN`. by @hildebrandmw in #1471 * [diskann-wide] Add `V4::new_checked_miri_uncached`. by @hildebrandmw in #1480 * [diskann-wide] Add maddubs intrinsic wrappers to V3 and V4 by @partychen in #1479 * Bump the github-actions group across 1 directory with 5 updates by @dependabot[bot] in #1482 * [benchmark-runner] Simplify Input representation by @hildebrandmw in #1475 * [diskann-quantization] Optimize spherical encoding scale search by @xinyuwen2 in #1478 * [diskann-garnet] Implement XVCREATE, XVIMPORT by @metajack in #1455 * [diskann-garnet] Support configurable start points and allow importing start points by @metajack in #1464 * [diskann-garnet] Add spherical 2-bit and 4-bit quantization by @metajack in #1465 * [deps] Address depandabot issue 32 by @hildebrandmw in #1496 * [diskann-disk] Preserve virtual start point out-edges when writing the disk index by @doliawu in #1483 * [diskann-quantization] Add Neon F32 Minmax Kernels by @pfoxARM in #1431 * [diskann-garnet] Add macOS arm64 support to NuGet publishing by @metajack in #1495 * [diskann-disk] Require start points at index initialization. by @hildebrandmw in #1497 * Expose metric-aware quantizer centroid distance by @cherryrhea in #1395 * [diskann-inmem] Add support for Product Quantization by @hildebrandmw in #1469 * Adds aarch64 prefetching into diskann-vector and diskann-inmem by @pfoxARM in #1408 * [diskann-inmem] Add scalar quantization by @hildebrandmw in #1484 * Bump the github-actions group with 3 updates by @dependabot[bot] in #1505 * [diskann-inmem] Spherical Quantization Round 2 by @hildebrandmw in #1485 * [diskann-quantization] Add SphericalQ 3-bit support and AVX2 kernel by @xinyuwen2 in #1474 * [muilt-vetor] Add standalone MinMax8 x MinMax4 MaxSim kernel by @partychen in #1494 ## New Contributors * @cherryrhea made their first contribution in #1395 **Full Changelog**: v0.60.0...v0.61.0 Co-authored-by: Mark Hildebrand <mhildebrand@microsoft.com>


Why
N × 0matrix layouts are valid and haveNlogical rows, but the mutable parallel row and window iterators previously relied on chunking the empty backing slice. Slice chunking cannot recover the logical row count from zero stored elements, so these iterators rejected zero-column matrices.What
Introduce an index-based
ParMuthelper that carries the matrix pointer, validated layout, and exclusive borrow lifetime across Rayon workers.par_rows_mut()andpar_window_iter_mut()now create disjoint mutable views from unique row indices and row ranges. This:The unsafe disjoint-view construction is covered by focused tests and strict-provenance Miri runs under both Stacked Borrows and Tree Borrows.
Benchmarks
There is no evidence that the index-based
ParMutimplementation introduces a performance regression for matrices with nonzero dimensions.I ran the benchmark 10 times, comparing the original chunk-based implementation (
baseline) with the index-based implementation (current).Midpoint estimates
Stability summary
Comparison
5×100, the ratio of medians is +3.2%. Paired run differences ranged from −9.1% to +9.5%, changed direction across runs, and averaged +1.7%.1M×100, the ratio of medians is −2.2%. The current implementation was faster in 7 of 10 paired runs, and the paired differences averaged −0.8%.Conclusion
The measured differences are within the observed run-to-run variability. There is no evidence that the index-based
ParMutimplementation introduces a performance regression.The benchmark code snippet for reproducibility: