Skip to content

Support zero-column mutable matrix iterators - #1462

Merged
Alex Razumov (arrayka) merged 9 commits into
mainfrom
u/arrayka/zero_column_iterators
Oct 13, 2026
Merged

Alex Razumov (arrayka) merged 9 commits into
mainfrom
u/arrayka/zero_column_iterators

Conversation

@arrayka

@arrayka Alex Razumov (arrayka) commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Why

N × 0 matrix layouts are valid and have N logical 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 ParMut helper that carries the matrix pointer, validated layout, and exclusive borrow lifetime across Rayon workers.

par_rows_mut() and par_window_iter_mut() now create disjoint mutable views from unique row indices and row ranges. This:

  • supports zero-column matrices without a special iterator branch;
  • yields one empty mutable slice per logical row;
  • yields correctly sized zero-column windows;
  • uses the same index-based structure as the immutable parallel iterators; and
  • preserves the existing behavior for matrices with nonzero dimensions.

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 ParMut implementation 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

Run 5×100 baseline 5×100 current 1M×100 baseline 1M×100 current
1 106.370 µs 105.500 µs 9.413 ms 9.091 ms
2 103.180 µs 107.850 µs 9.441 ms 9.814 ms
3 117.450 µs 106.820 µs 9.210 ms 8.832 ms
4 108.880 µs 108.190 µs 9.467 ms 8.888 ms
5 102.080 µs 110.940 µs 8.546 ms 8.698 ms
6 102.990 µs 95.931 µs 9.068 ms 8.895 ms
7 105.220 µs 108.590 µs 9.282 ms 8.998 ms
8 104.830 µs 111.700 µs 9.052 ms 9.026 ms
9 107.640 µs 109.640 µs 8.890 ms 9.458 ms
10 102.650 µs 112.430 µs 9.216 ms 9.083 ms

Stability summary

Shape/implementation Median MAD CV Range
5×100 baseline 105.025 µs 2.205 µs 4.30% 102.080–117.450 µs
5×100 current 108.390 µs 2.060 µs 4.35% 95.931–112.430 µs
1M×100 baseline 9.213 ms 0.180 ms 3.10% 8.546–9.467 ms
1M×100 current 9.012 ms 0.120 ms 3.62% 8.698–9.814 ms

Comparison

  • For 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%.
  • For 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%.
  • All four result sets have a CV between 3.1% and 4.4%.

Conclusion

The measured differences are within the observed run-to-run variability. There is no evidence that the index-based ParMut implementation introduces a performance regression.

The benchmark code snippet for reproducibility:

  group.bench_function(BenchmarkId::new("current", nrows), |b| {
      let mut matrix = Owned::from_element(nrows, NCOLS, 0u32);
      b.iter(|| {
          update_rows(black_box(&mut matrix).par_rows_mut());
          black_box(matrix.as_slice());
      });
  });

fn update_rows<'a, I>(rows: I)
where
    I: ParallelIterator<Item = &'a mut [u32]>,
{
    rows.for_each(|row| {
        let (first, rest) = row.split_first_mut().unwrap();
        *first = first.wrapping_add(1);

        let mut previous = *first;
        for value in rest {
            *value = value.wrapping_add(previous).wrapping_add(1);
            previous = *value;
        }
    });
}

Preserve logical rows and windows for N x 0 matrices during parallel mutable iteration instead of panicking.
@arrayka
Alex Razumov (arrayka) requested a balanced review from Copilot October 5, 2026 22:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 High severity · 2 Medium severity

Open (3)
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 ZeroColumnMut helper to manufacture empty mutable row/window views for zero-column matrices (Rayon-only).
  • Update MatrixMut::{par_rows_mut, par_window_iter_mut} to branch on ncols == 0 and return appropriate parallel iterators via Either.
  • 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.

Comment thread diskann-utils/src/views/rowmajor/iter.rs Outdated
Comment thread diskann-utils/src/views/rowmajor.rs
Comment thread diskann-utils/src/views/rowmajor/iter.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-commenter

Codecov Comments Bot (codecov-commenter) commented Oct 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.98%. Comparing base (31589c3) to head (f71f9c1).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
miri 91.98% <100.00%> (+0.07%) ⬆️
unittests 91.92% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
diskann-utils/src/views/rowmajor.rs 96.93% <100.00%> (+0.04%) ⬆️
diskann-utils/src/views/rowmajor/iter.rs 100.00% <100.00%> (ø)

... and 40 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arrayka
Alex Razumov (arrayka) marked this pull request as ready for review October 5, 2026 23:08
@arrayka
Alex Razumov (arrayka) requested a review from a team October 5, 2026 23:08
Alex Razumov (from Dev Box) added 3 commits October 6, 2026 11:41
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.
Comment thread diskann-utils/src/views/rowmajor.rs Outdated
Alex Razumov (from Dev Box) added 2 commits October 7, 2026 13:12
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The concurrent unsafe pointer and lifetime handling warrants final human validation despite focused coverage.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment thread diskann-utils/src/views/rowmajor/iter.rs Outdated
Comment thread diskann-utils/benches/bench_main.rs Outdated
@arrayka
Alex Razumov (arrayka) enabled auto-merge (squash) October 12, 2026 20:03
@arrayka
Alex Razumov (arrayka) merged commit 5bc7321 into main Oct 13, 2026
31 checks passed
@arrayka
Alex Razumov (arrayka) deleted the u/arrayka/zero_column_iterators branch October 13, 2026 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants