From 87e81a17e921de6a54eb86d9752b781e75f66313 Mon Sep 17 00:00:00 2001 From: Benjamin Demaille Date: Tue, 11 Aug 2026 21:33:28 +0200 Subject: [PATCH 1/4] test+build: validate the LCP array, and pin the release profile Two prerequisites for the performance work that follows, neither of which changes behaviour. The crate had no test covering the LCP array. That is the riskiest possible gap for this algorithm: the public entry points discard the array, but the *next* merge level consumes it in the three-case decision, so a single wrong LCP entry silently reorders suffixes one level up and the SA comes out subtly wrong. Add four tests that check `lcp[0] == 0` and `lcp[i] == lcp(text[sa[i-1]..], text[sa[i]..])` against a naive oracle, over fixtures, random texts across four alphabet sizes, long runs and periodic text, and finite `max_context`. `bench/README.md` claims the published numbers were taken with fat LTO and one codegen unit, supplied by a parent workspace. That workspace is not in this repo, so every build made from it since the crate went standalone has used `lto = false, codegen-units = 16`. Pin the profile here. In a library crate `[profile.release]` applies only when this crate is the workspace root, so it affects this repo's own tests, examples and benches and is invisible to downstream consumers. Measured on Apple M4 Max, 12 threads, chr21 FASTA (47.5 MB): 27.8 s -> 24.0 s wall. Neutral on N-free DNA. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 13 ++++++ src/sample_sort.rs | 109 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 12261dc..4827078 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,19 @@ categories = ["algorithms", "data-structures"] repository = "https://github.com/COMBINE-lab/caps-sa" readme = "README.md" +# The published benchmark numbers in `bench/` were taken with fat LTO and a +# single codegen unit. That configuration used to come from a parent workspace +# that no longer exists in this repo, so pin it here. A `[profile.release]` in a +# library crate applies only when this crate is the workspace root — i.e. to +# this repo's own tests, examples and benches — and is invisible to downstream +# consumers, who keep their own profile. +[profile.release] +lto = "fat" +codegen-units = 1 + +[profile.bench] +inherits = "release" + [dependencies] rayon = "1" tempfile = "3" diff --git a/src/sample_sort.rs b/src/sample_sort.rs index daa9264..b2f0dc7 100644 --- a/src/sample_sort.rs +++ b/src/sample_sort.rs @@ -422,6 +422,115 @@ mod tests { assert_eq!(got, want, "mismatch on text {text:?}"); } + /// Run the production kernel and return **both** the suffix array and + /// the LCP array it computes as a byproduct. + /// + /// The public entry points discard the LCP array, but it is not an + /// incidental artefact: the next merge level *consumes* it in the + /// three-case decision, so a single wrong LCP entry silently reorders + /// suffixes at the level above. It therefore needs direct coverage. + fn build_sa_and_lcp(text: &[u8], max_ctx: usize) -> (Vec, Vec) { + let n = text.len(); + let mut sa: Vec = (0..n as u32).collect(); + let mut sa_w = vec![0u32; n]; + let mut lcp_arr = vec![0u32; n]; + let mut lcp_w = vec![0u32; n]; + merge_sort( + text, + &PlainText::new(n), + &mut sa, + &mut sa_w, + &mut lcp_arr, + &mut lcp_w, + max_ctx, + LcpDispatch::detect(), + ); + (sa, lcp_arr) + } + + /// Byte-at-a-time LCP of `text[a..]` and `text[b..]`, capped at `max_ctx`. + fn naive_lcp(text: &[u8], a: usize, b: usize, max_ctx: usize) -> usize { + let lim = (text.len() - a).min(text.len() - b).min(max_ctx); + (0..lim).take_while(|&i| text[a + i] == text[b + i]).count() + } + + /// Assert the LCP-array postcondition stated on [`merge_sort`]: + /// `lcp[0] == 0` and `lcp[i] == lcp(text[sa[i-1]..], text[sa[i]..])`. + fn assert_lcp_valid(text: &[u8], max_ctx: usize) { + let (sa, lcp) = build_sa_and_lcp(text, max_ctx); + if sa.is_empty() { + return; + } + assert_eq!(lcp[0], 0, "lcp[0] must be 0 (text {text:?})"); + for i in 1..sa.len() { + let want = naive_lcp(text, sa[i - 1] as usize, sa[i] as usize, max_ctx); + assert_eq!( + lcp[i] as usize, want, + "lcp[{i}] wrong for sa[{}]={} vs sa[{i}]={} (text {text:?})", + i - 1, + sa[i - 1], + sa[i], + ); + } + } + + #[test] + fn lcp_array_matches_naive_on_fixtures() { + for text in [ + b"banana".as_slice(), + b"mississippi", + b"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + b"abababababababababababababab", + b"a", + b"", + ] { + assert_lcp_valid(text, usize::MAX); + } + } + + #[test] + fn lcp_array_matches_naive_on_random() { + use rand::{RngExt, SeedableRng}; + let mut rng = rand::rngs::StdRng::seed_from_u64(0x1CB0); + for &sigma in &[2u8, 4, 6, 255] { + for &n in &[2usize, 3, 7, 16, 17, 63, 64, 65, 200, 1000, 5000] { + let text: Vec = (0..n).map(|_| rng.random_range(0..sigma)).collect(); + assert_lcp_valid(&text, usize::MAX); + } + } + } + + /// Long runs of one symbol are the worst case for the LCP invariant: + /// adjacent suffixes share almost everything, so every `lcp[i]` is + /// large and an off-by-one is easy to miss. + #[test] + fn lcp_array_on_long_runs_and_periodic_text() { + assert_lcp_valid(&vec![7u8; 2000], usize::MAX); + let periodic: Vec = (0..2000).map(|i| (i % 3) as u8).collect(); + assert_lcp_valid(&periodic, usize::MAX); + // A run embedded in noise, the shape a poly-N genome block has. + let mut mixed: Vec = (0..500).map(|i| (i % 4) as u8).collect(); + mixed.extend(std::iter::repeat_n(4u8, 1500)); + mixed.extend((0..500).map(|i| (i % 4) as u8)); + assert_lcp_valid(&mixed, usize::MAX); + } + + #[test] + fn lcp_array_respects_max_context() { + use rand::{RngExt, SeedableRng}; + let mut rng = rand::rngs::StdRng::seed_from_u64(0xC7A); + for &max_ctx in &[1usize, 2, 4, 16] { + for &n in &[64usize, 500] { + let text: Vec = (0..n).map(|_| rng.random_range(0..3u8)).collect(); + let (sa, lcp) = build_sa_and_lcp(&text, max_ctx); + for i in 1..sa.len() { + let want = naive_lcp(&text, sa[i - 1] as usize, sa[i] as usize, max_ctx); + assert_eq!(lcp[i] as usize, want, "lcp[{i}] wrong with max_ctx={max_ctx}"); + } + } + } + } + #[test] fn empty_text() { let sa: Vec = build_in_memory::(&[]); From b90b3c5fd4ef0cf33e278d0ae922e2b382069350 Mon Sep 17 00:00:00 2001 From: Benjamin Demaille Date: Tue, 11 Aug 2026 21:33:42 +0200 Subject: [PATCH 2/4] ci: add a Rust workflow The repository had no Rust CI at all: the only workflow deployed the docs site, while 73 tests sat in the tree with nothing running them. That is not a safe baseline for changing the sorting kernel. Covers both architectures that matter here, since the LCP kernel and the pooled external-memory bucket path are the parts that diverge per platform: macOS is aarch64/NEON, Ubuntu is x86_64/AVX2. Runs the tests in debug as well as release, because debug is what exercises the `debug_assert`s guarding the unchecked scatter in `radix.rs` and the buffer-length invariants in the merge kernel. Adds fmt, clippy with warnings denied, a check against the declared 1.89 MSRV, and rustdoc with broken links denied. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 69 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..0567d90 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,69 @@ +name: CI + +on: + push: + branches: [main] + pull_request: + +env: + CARGO_TERM_COLOR: always + RUSTFLAGS: -D warnings + +jobs: + test: + name: test (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + # macOS is aarch64/NEON, Ubuntu is x86_64/AVX2. The LCP kernel and the + # pooled external-memory bucket path are the parts that differ per + # platform, so both need to run. + os: [ubuntu-latest, macos-latest] + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + + # Debug catches the `debug_assert`s that guard the unchecked scatter in + # `radix.rs` and the buffer-length invariants in the merge kernel. + - name: Test (debug) + run: cargo test --all-targets + - name: Test (release) + run: cargo test --release --all-targets + - name: Doc tests + run: cargo test --doc + + lint: + name: fmt + clippy + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt, clippy + - uses: Swatinem/rust-cache@v2 + - run: cargo fmt --all --check + - run: cargo clippy --all-targets -- -D warnings + + msrv: + name: MSRV (1.89) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + # Pinned to the `rust-version` in Cargo.toml, which is set by the + # stabilised AVX-512 intrinsics the LCP fast path uses. + - uses: dtolnay/rust-toolchain@1.89 + - uses: Swatinem/rust-cache@v2 + - run: cargo check --all-targets + + docs: + name: rustdoc + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + - run: cargo doc --no-deps + env: + RUSTDOCFLAGS: -D warnings From c3c454d4e9a44840cb3be6a452969ad823ec9c46 Mon Sep 17 00:00:00 2001 From: Benjamin Demaille Date: Tue, 11 Aug 2026 21:33:42 +0200 Subject: [PATCH 3/4] docs: fix a broken intra-doc link in `build_ext_mem_for_filter` Public documentation linked to `FilteredSource`, which is private, so `cargo doc` fails under `RUSTDOCFLAGS=-D warnings`. Pre-existing, but it blocks the rustdoc job added in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) --- src/ext_mem.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ext_mem.rs b/src/ext_mem.rs index 621a3ab..1180273 100644 --- a/src/ext_mem.rs +++ b/src/ext_mem.rs @@ -388,8 +388,8 @@ where /// bytes — ~770 MB on the human genome, vs the ~50 GB the equivalent /// `Vec` would take). Phase 1's per-subarray fill is then driven /// by popcount-walking the bitmap; the predicate is **never invoked -/// again** after the initial build. See [`FilteredSource`] for the -/// memory accounting and the inner loop. +/// again** after the initial build. See the crate-internal +/// `FilteredSource` for the memory accounting and the inner loop. /// /// Use this entry when the caller already has the text in RAM and /// the kept positions are described by a cheap per-position From 7d0ac51a5a79872936b8d48d114c4ea1376bef2b Mon Sep 17 00:00:00 2001 From: Benjamin Demaille Date: Wed, 12 Aug 2026 09:21:52 +0200 Subject: [PATCH 4/4] ci: run the workflow on every branch A pull request opened from a fork by a first-time contributor does not run workflows until a maintainer approves them, which is why the API showed no check runs at the tips reviewed in #7. Building on push means the contributor's own fork produces evidence that can be linked from the PR. Adds `workflow_dispatch` for the same reason. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 7 ++++++- src/sample_sort.rs | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0567d90..34f36e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,9 +1,14 @@ name: CI on: + # Every branch, not just `main`. A pull request opened from a fork by a + # first-time contributor does not run workflows until a maintainer approves + # them, so `pull_request` alone leaves reviewers with no check runs to look + # at. Building on push means the contributor's own fork produces evidence + # that can be linked from the PR. push: - branches: [main] pull_request: + workflow_dispatch: env: CARGO_TERM_COLOR: always diff --git a/src/sample_sort.rs b/src/sample_sort.rs index b2f0dc7..882bc27 100644 --- a/src/sample_sort.rs +++ b/src/sample_sort.rs @@ -465,7 +465,8 @@ mod tests { for i in 1..sa.len() { let want = naive_lcp(text, sa[i - 1] as usize, sa[i] as usize, max_ctx); assert_eq!( - lcp[i] as usize, want, + lcp[i] as usize, + want, "lcp[{i}] wrong for sa[{}]={} vs sa[{i}]={} (text {text:?})", i - 1, sa[i - 1], @@ -525,7 +526,10 @@ mod tests { let (sa, lcp) = build_sa_and_lcp(&text, max_ctx); for i in 1..sa.len() { let want = naive_lcp(&text, sa[i - 1] as usize, sa[i] as usize, max_ctx); - assert_eq!(lcp[i] as usize, want, "lcp[{i}] wrong with max_ctx={max_ctx}"); + assert_eq!( + lcp[i] as usize, want, + "lcp[{i}] wrong with max_ctx={max_ctx}" + ); } } }