Skip to content

perf(fragindex, extract): a per-scan probe scratch, and a reverted hoist that did not pay - #114

Merged
RobbinBouwmeester merged 3 commits into
mainfrom
perf/fragindex-final
Sep 23, 2026
Merged

perf(fragindex, extract): a per-scan probe scratch, and a reverted hoist that did not pay#114
RobbinBouwmeester merged 3 commits into
mainfrom
perf/fragindex-final

Conversation

@RobbinBouwmeester

@RobbinBouwmeester RobbinBouwmeester commented Sep 22, 2026

Copy link
Copy Markdown
Member

Two items the accumulator rewrite was blocked on, because they live in matchers/fragindex.rs. One pays, one was measured and reverted, and the reverting is the useful part.

What survived

A per-scan (f64, u32) setup scratch. The probe's bin index costs a ln(), and the mass-offset correction a division; both were recomputed per sub-range task for the same peaks. The scratch computes them once per scan into a task-local buffer. It has the same ln() count as before and pays by taking the ln() off the probe's dependency chain, so it needs no shared buffer, no extra memory and no serial pass. It carries the corrected m/z as well, because a bin-only scratch turned the loess-grid arm from −1.7% into +5.6%: factor_at is 8.3 ns per peak there against 0.725 for the scalar case.

shape this scratch
16 windows, 4 tasks each, the default −11.7 / −6.4%
16 windows, 1-2 tasks each −15.1 / −17.9%
one wide window, 49 tasks −17.4 / −18.6%

End to end on a real AIF extraction, −0.3 to −4.5% of the accumulation phase.

emit_range slices and zips the four posting arrays instead of indexing each per posting: −9.3 / −5.7 / −6.3 / −6.7% of the verify loop at 1, 8, 64 and 512 postings. The verify loop is about 11% of the widest probe, so this is under 1% end to end, below what the A/B resolves. Kept because it is free, not because it moves the stage. The ppm predicate is untouched.

What was reverted, and why that matters

The first version shared a precomputed bin buffer across a window's tasks and reported −28 to −31%. The review showed the benchmark built that buffer outside the timer while the arm it was compared against filled its scratch inside one, and that the harness fed unsorted peaks where production peaks are m/z sorted. Re-measured honestly, with every arm paying for what it uses:

shape buffer, filled serially buffer, filled on the pool
the default shape +8.5 / +22.1% −16.6 / −8.5%
16 windows, 1-2 tasks +38.9 / +30.0% −10.3 / −15.3%

The serial fill runs on the calling thread before the rayon scope, so the pool idles through one pass over the in-flight peaks: 39.6M × 5.4 ns = 430 ms of a 3.3 s phase. End to end on the real AIF run that is +14.0% on the accumulation phase. Break-even needs tasks_per_window >= threads, which the default shape is nowhere near. Filling on the pool removes the regression and still loses to the plain scratch on two of three shapes, for 4 bytes per in-flight peak. So the buffer, its per-scan offsets, its Option branch and its debug assertions are gone, along with a memory claim that was off by about ten times.

Equality

Byte-identical on real data rather than only on the fixture: 69 AIF extract runs across three binaries wrote the same psms_extracted.parquet and chromatograms.parquet, 41,677 accepted every time, which unlike the fixture actually exercises the split-task path. ci/smoke.sh prints SMOKE_OK with 127 artifacts byte-identical to a parent-commit binary.

The benchmark is committed as #[ignore]d tests so the numbers can be re-derived instead of re-argued.

🤖 Generated with Claude Code

RobbinBouwmeester and others added 3 commits September 22, 2026 21:38
@RobbinBouwmeester @claude
perf(fragindex, extract): a precomputed-bin probe, and slice the post…
0f5310e 
…ing arrays

Two items the extract rewrite could not reach, because they live in
`fragindex.rs`.

(1) `FragIndex::probe_peak_win_binned` takes the bin the probe would have computed,
and `bin_of` computes it; `probe_peak_win` is now that function with `bin_of`
applied, so the three cannot drift. The bin is a `ln()`, and extract's sub-range
tasks each re-walk every scan of their window, so each task recomputed it for every
peak: up to twice the thread count, about eight times per window in a grouped band
search. `accumulate_groups` now computes a window's bins once into a flat `u32`
buffer with one offset per scan, wherever the window is actually split into more
than one task, and into a task-local per-scan scratch otherwise.

The other half of the hoist, `peak.mz / mass_off.factor_at(peak.mz)`, is
deliberately still recomputed per task. The previous author measured it alone as a
loss (8 B/peak, +200 MB of peak RSS on the AIF fixture and 317 MB with the whole run
in one batch, no measurable time) and that reading holds: on the probe
microbenchmark added here, five rounds, buffering m/z AND bin was -25 to -27 / -9 to
-14 / -2 to -7% and the bin alone -20 to -29 / -9 to -17 / -4 to -6% (narrow /
medium / wide candidate window), which is one spread, for three times the buffer.
The division is 0.72 ns/peak; the `ln()` is 4.99.

So the buffer is 4 B/peak of the windows in flight: a sixteenth of the run under the
default `windows_in_flight`, a band's 1-3 windows under `groups.parallel`. Measured
against the shipped probe, min of 12 per round, five rounds: the split window (the
shared buffer) -28 to -31 / -8 to -17 / -5 to -7%, and the unsplit window (scratch
only, the same number of `ln()` calls) -15 to -27 / -4 to -11 / -3 to +1%. The
scratch pays because a separate pass takes the `ln()` off the dependency chain the
bin-cache load and then the posting loads hang from. It is also why the bins reach
the peak loop as a `&[u32]` in both cases rather than as an `Option` tested per
peak: that branch measured +4 to +7% on the wide arm even always taking one side,
more than the `ln()` it saved.

(2) `emit_range` slices the four posting arrays once and zips them, so the loop pays
four range checks for a whole range instead of four per posting, and returns early
on the empty range a narrow candidate window mostly produces. Paired A/B on one host
with two binaries differing only in this: -3 to -4.4% of the probe where the ranges
are long (8 and 65 emitted postings per peak), flat where they are not. The
`within_ppm` call is untouched, as the brief asks: precomputing m/z bounds from
`peak_mz` rounds differently from the per-pair `hi - lo <= tol * 1e-6 * lo`, and the
tolerance edge decides real matches.

Risk to output equality: none observed, and the shapes are argued rather than only
sampled. The bin is a pure function of `q_mz`, and `q_mz` is recomputed in the task
from the same peak and the same offset the buffer used, so the buffered bin is the
same value; extract debug-asserts that per peak. `emit_range` keeps the same
predicate, the same order and the same four columns per posting.

Measured, not assumed: `ci/smoke.sh` prints SMOKE_OK, and every one of the 29 data
artifacts it writes is byte-identical to the same fixture run through a binary built
from the parent commit (only `*.report.json` `elapsed_ms` and the manifest's paths /
timings / git sha differ). The fixture cannot resolve the timing difference: it is
IO-dominated, its extract stage is milliseconds, and its eight windows on two threads
never split, so the shared-buffer path does not even run there. That is what the
microbenchmark is for, and it is committed as an `#[ignore]`d test rather than left
in a comment.

Tests: `bin_of_is_the_bin_the_probe_uses` pins the equality the whole contract rests
on, over clamped and out-of-range m/z; the binned probe is compared callback-for-
callback against `probe_peak` inside the existing drop-in test;
`a_bin_past_the_top_is_clamped_not_a_panic` pins the defensive clamp;
`emit_range_pairs_every_posting_with_its_own_columns` pins that every within-
tolerance posting is emitted exactly once carrying its own candidate, intensity and
ordinal, over a run long enough for a mis-aligned slice to show, which a count-only
assertion on a three-posting fixture would not. The extract side is pinned by the
existing `candidate_range_split_reproduces_the_unsplit_accumulation`, which already
runs the same fixture at two threads (unsplit, now the scratch path) and at eight and
sixteen (split, now the shared buffer) and compares the accumulation hit for hit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RobbinBouwmeester @claude
perf(fragindex, extract): re-measure the bin hoist, revert the shared…
817d332 
… buffer

An adversarial review said the -28 to -31% in 0f5310e could not be trusted and
that the change was probably a wall-clock LOSS on the default shape. It was
right on both counts. This commit fixes the harness, re-measures, reverts the
per-window buffer, and keeps the two parts that survive.

WHY THE OLD NUMBER WAS WRONG

Three separate defects, all in the direction that flattered the change.

1. The arms were not comparable. The shared bin buffer was built at
   `bench_fragindex.rs:251`, OUTSIDE every timer, while the arm it was compared
   against filled its scratch inside one. The buffer arm read a fill it never
   paid for.
2. The harness modelled the wrong peak stream: uniform random m/z in arbitrary
   order, where production peaks are m/z sorted (`Ms2Scan.peaks`, and the
   extract loop walks them in that order). Sorted input is exactly the cache
   behaviour the claimed mechanism depends on. Re-measured sorted, the medium
   arm's probe is 168.6 ns/peak, not 257.9.
3. Nothing measured wall clock. The buffer was filled SEQUENTIALLY on the
   calling thread before `rayon::scope`, and the per-task work it removed had
   been spread across the pool. A per-probe microbenchmark cannot see that
   trade at all.

THE CORRECTED MEASUREMENT

`tests/bench_fragindex.rs` now has two parts, peaks m/z sorted within a scan,
checksums asserted equal across every arm. `bench_probe` times one thread and
charges every arm its own setup inside its own timer (the buffer's fill is timed
separately and charged back, divided by the tasks that read one fill, which is
the only honest way to do it serially). `bench_wall` is new: it rebuilds
`accumulate_groups`' own task shape from its own arithmetic and runs it over
rayon, which is the unit that decides a serial-for-parallel trade.

`bench_wall`, 32 threads, min of 9, two independent passes, against the
pre-commit in-probe baseline:

  shape                        scratch          buffer, serial   buffer, on pool
  16 windows, 4 tasks each   -11.7 / -6.4%    +8.5 / +22.1%     -16.6 / -8.5%
  16 windows, 1-2 tasks      -15.1 / -17.9%   +38.9 / +30.0%    -10.3 / -15.3%
  one wide window, 49 tasks  -17.4 / -18.6%   -23.7 / -19.0%    -26.2 / -22.1%

The first row is the default configuration: `tasks_per_window` is
`(2 * threads).div_ceil(groups.len())`, which is 4 at 32 threads and 16 windows
in flight, so the fill costs one serial pass per peak to save three thirty-
seconds of one. Break-even needs `tasks_per_window >= threads`, i.e. a batch of
one or two windows, which only the all-ion row is.

End to end, `mumdia extract` on the real AIF run (152 windows, 1.69M candidates,
465,806 MS2 scans, 32 threads, three binaries interleaved over ~24 reps each,
timing the accumulation phase between the two log lines that bracket it so the
310 MB spectra read is not in the number). Against the pre-commit baseline's
3,274 ms min / 3,360 mean-of-3-fastest / 4,233 median:

  shared buffer, serial fill   +14.0 / +13.2 / +9.3%
  per-scan scratch              -0.3 /  -1.9 / -4.5%

The regression is the size the model predicts: one serial pass over the
in-flight peaks is 39.6M x 5.4 ns = 430 ms of a 3.3 s phase. The machine is
noisy (reps spread 2.5x, hybrid CPU), so the sign and the ordering are the
result; they are the same in every statistic of every pass.

REVERTED

- The per-window `peak_bins` buffer and `per_window`, with the `Option` branch,
  the per-scan offsets, and the two `debug_assert`s that were the only thing
  standing between a buffer/scan mismatch and silently dropped hits in release.
  Also gone with it: the second evaluation of `peak.mz / factor_at(peak.mz)`
  that the two sites had to keep textually identical forever.
- The memory claim. "4 bytes per peak of the windows in flight: a sixteenth of
  the run" was wrong twice: the in-flight fraction is
  `windows_in_flight / n_windows` (16 of 152 on AIF, about 17 MB there, but the
  whole run on an acquisition with 16 windows or fewer, and one copy per band
  under `groups.parallel`). Measured, peak RSS did not separate the arms at all
  (2,266-2,499 MB across all three binaries).
- "-4 to -6% of the probe" for `emit_range`. It came from a pair of binaries
  nothing could reproduce, and the harness it cited could not produce it because
  every arm calls the same private `emit_range`.

KEPT

- The per-scan setup scratch, which is where the whole gain was. `Vec<(f64, u32)>`
  holding ONE scan's `(q_mz, bin)`, a few KB, task-local, refilled per scan. It
  makes exactly as many `ln()` calls as the probe did; it pays because a separate
  pass takes the `ln()` off the dependency chain the bin-cache load and then the
  posting loads hang from. Nothing is shared, nothing is built before the pool
  starts, no thread waits, and no memory is added. It carries `q_mz` as well as
  the bin, so `factor_at` runs once per peak as it did before: that is free at
  this size and matters under `search_seed.mass_cal_loess`, where `factor_at` is
  a binary search over a ~74-point grid at 8.3 ns/peak against 0.725 scalar -- a
  bin-only scratch turned that arm from -1.7% into +5.6%.
- `bin_of` / `probe_peak_win_binned`, which the scratch needs. Their doc comments
  now say what they are for (compute the setup in a pass of its own) and what
  they are not for (caching bins across calls).
- `emit_range`'s slice-and-zip and its empty-range early return, measured
  SEPARATELY as the review asked. `bench_emit_range` (new, `#[ignore]`d, inside
  `mod tests` where it can reach the private fn) times it against a verbatim copy
  of the indexed loop it replaced, same binary, same index, checksum asserted
  equal: -9.3 / -5.7 / -6.3 / -6.7% at 1, 8, 64 and 512 postings in the range.
  That is a percentage of the verify loop, which is about 11% of the widest
  probe, so the ceiling end to end is under 1% and the AIF A/B cannot resolve it.
  Kept for being free and never slower, not for moving the stage.

Also: `bin_of_is_the_bin_the_probe_uses` no longer claims to be the test the
whole contract rests on (it is `f(x) == f(x)` bar a u32 cast); the equality that
matters is now structural, since the scratch computes `q_mz` and its bin in one
expression and the probe reads both out of one entry. The vacuous
`assert_eq!(a.len(), got.len())` in the emit_range test now dedups first.
docs/06 line references were half-stale before this branch and are all
refreshed.

OUTPUT EQUALITY

Byte-identical, verified rather than argued, on real data and on the fixture.
All 69 AIF extract runs across the three binaries wrote the same
`psms_extracted.parquet` and `chromatograms.parquet` (41,677 accepted in every
one) -- which, unlike `ci/smoke.sh`, actually exercises the split-task path.
`ci/smoke.sh` prints SMOKE_OK and 127 of its artifacts are byte-identical to a
binary built from the parent commit (the 5 that differ are logs and a
work-directory path inside `planted.json`).

Validation: `cargo fmt --check`, `cargo clippy --workspace --all-targets
-D warnings`, `cargo test --workspace` (314 passed, 1 ignored),
`cargo build --release --locked`, `ci/smoke.sh` SMOKE_OK,
`python ci/check_doc_refs.py` OK.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RobbinBouwmeester @claude
docs(config): regenerate for the moved source lines
6d97e68 
The reference records by source line the environment reads whose name is not a
literal, and the probe scratch moved them in extract.rs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RobbinBouwmeester RobbinBouwmeester mentioned this pull request Sep 23, 2026
Merged
@RobbinBouwmeester
RobbinBouwmeester merged commit 03e17e8 into main Sep 23, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@RobbinBouwmeester