Repository navigation
Support zero-column mutable matrix iterators - #1462
Alex Razumov (arrayka) wants to merge 5 commits into
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… · 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% 90.88% -1.02%
==========================================
Files 582 583 +1
Lines 114947 115937 +990
==========================================
- Hits 105640 105374 -266
- Misses 9307 10563 +1256
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.
| matrix.window(window_nrows) | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
After thinking about it more, I still find the use of Either to be deeply unsatisfying. Since ZeroColumnMut is already messing around with some unsafety, what if we went the way of par_rows and par_window_iter and did something like
struct ParMut<'a, T> {
ptr: NonNull<T>,
layout: Layout<T>,
_lifetime: PhantomData<&'a mut T>,
}
impl ParMut<'a, T> {
fn row_disjoint_unchecked(&self, row: usize) -> &'a mut [T];
fn window_disjoint_unchecked(&self, rows: Range<usize>) -> Mut<'a, T>;
}If we use index based parallel iterators, we get the necessary disjointedness.
Why
N x 0matrix layouts are valid, but parallel mutable row and window iterators panic instead of preserving the logical shape.What
Yield empty mutable rows and correctly sized zero-column windows, with regression and strict-provenance Miri coverage.
Benchmarks
There is no evidence that
Eitherintroduces a performance regression for the common case (MxN matrices with M an N > 0).I re-run the benchmarks 10 times - before/after numbers are similar:
Midpoint estimates
Stability summary
* The
beforeimplementation’s CV is distorted by run 8. Criterion marked two samples in that measurement as severe low outliers, producing a very broad33.907–88.258 µsconfidence interval.Conclusion
Eitherintroduces a performance regression.