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!


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: