perf: share a run's spectra, fit the mass calibration once, and shrink a decoded peak from 24 bytes to 8 - #116
Merged
Merged
Conversation
…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>
…iations
A grouped search learned a fragment tolerance 35% too wide, and it cost
identifications. Six-file HYE Astral, same library and the same retention-time
model, 100 isolation-window bands against none:
unbanded 100 bands
frag_ppm_offset -1.8486 -1.8834
frag_ppm_sigma 8.452 11.400
n_dev 181,196 200,257
ppm_residual_mad 0.907 1.035
candidates accepted 4,986,153 6,609,984
peptides at 1% 113,860 110,006
precursors at 1% 126,436 121,966
Causal rather than correlated: one band extracted twice, identical in every input
but the calibration file it was handed, accepted 12,414 candidates at 11.40 ppm and
8,791 at 8.45; an unbanded control on the same adapted library returned the unbanded
arm's 113,860 peptides exactly.
search-seed fits 1.5 * p95(|dev - median|) over the ppm deviations of the matched
fragments of its confident targets. Each band did that on its own ~2,000 deviations
and seed-pool combined the bands' SCALARS, which is not that estimator: a small
sample's p95 has a heavier tail than the union's, and each band selected calibrants
on its OWN q (106,088 band-confident against 97,584 pooled).
So fit once over the union, as the retention-time calibration already does with the
pooled anchors under groups.calibration = global:
- the estimator, the masscal.json body and the calibrant sidecar move into
masscal.rs, so the two call sites share one function rather than two copies of the
percentile logic;
- search-seed, when asked (SearchSeedParams::emit_calibrants, set only by the
grouped path), writes <seed>.masscal.parquet: library-wide candidate_id,
scan_index, frag_mz, ppm, one row per matched fragment, 16 B each;
- seed-pool keeps the deviations whose PSM the POOLED q accepts at fdr_seed, and
whose scan the pool kept, so an overlap candidate contributes one PSM's fragments
exactly as an ungrouped seed would, then fits MassCal::fit_from once.
two_pass_mass_cal and the mass_cal_loess grid keep working on this path because
both read the deviations rather than a scalar;
- a missing sidecar, which is a band directory from an older build, keeps today's
scalar combination and says so in the log, with a second warning when
mass_cal_loess cannot be honoured from scalars. masscal.json records which path
ran in masscal_source.
The band q is not the pooled q in either direction, and where it is STRICTER the
pooled filter alone would starve the fit: on the CI fixture at three bands every
band's own q rejects every one of its targets, so a strictly q-selected sidecar is
empty in all three. The sidecar therefore also offers the band's best-scoring
targets down to masscal::CALIBRANT_OFFER_PSMS (2,000) and lets the pooled q choose.
The three bands then contribute 437 + 401 + 230 = 1,068 deviations and the pool fits
frag_tol_ppm 5.0 at offset 0.0, which is to the digit what the ungrouped search of
the same spectra produces. Before this the same run calibrated nothing and extracted
at the configured 20 ppm.
What stays identical: an ungrouped run. emit_calibrants is false at all three
ungrouped call sites, no sidecar is written and the calibrant selection is
unchanged. Verified by running ci/smoke.sh from this tree and from a816768 and
comparing all 223 output files: every artifact byte-identical, only elapsed_ms,
git_sha and embedded work-directory paths differ.
What deliberately moves: a grouped run's mass calibration, and every artifact
downstream of it (extract, features, compete, rescore, quant, report). That is the
point of the change.
Validated from rust/mumdia: cargo fmt --check, cargo clippy --workspace
--all-targets -D warnings (clean), cargo test --workspace (319 lib tests), plus
ci/smoke.sh (SMOKE_OK) and a three-band grouped run on the fixture.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… 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>
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>
Both branches added a field to SearchSeedParams: the lent MS2 buffer and the calibrant-deviation switch. Both are kept at every call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note above the per-peak setup is merged rather than taken from one side: the fragment-index branch's version records both attempted hoists and what they measured, and the shared-spectra branch's version records that the memory-cheap variant of the hoist, writing the corrected m/z back into the scan, is now forbidden because the scans are one buffer lent to every band. Both belong. The two seed call sites in tests/pipeline.rs are ungrouped in shape, so they emit no calibrant sidecar. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The decoded peak array is the engine's largest resident buffer, one entry per
MS2 point of the run, and since the shared-spectra change one copy of it is
resident for a whole grouped run. Two thirds of it were empty.
- `Peak.ion_mobility: Option<f32>` was 8 of the 24 bytes (an `f32` has no niche,
so the `Option` costs a full word) and was written `None` at three sites and
read at zero, workspace-wide. No artifact column carries it; `Peak` is not
serialised. Deleted. A 4D run wants a per-scan `Vec<f32>` parallel to `peaks`,
the shape `Ms1Scan` already uses, not a per-peak always-`None` `Option`.
- `Peak.mz: f64` was widened at load from the `f32` the spectra artifact stores,
so the f64 held nothing the f32 did not. Narrowed to `f32`; the ~22 read sites
widen with `as f64` at the comparison instead.
- `Ms2Scan.id: String` was decoded at load and read by nothing but a
`debug_assert` fingerprint and one test assertion. Deleted; the `id` column
stays in the artifact and scans are addressed by `scan_index`.
Measured sizes, not inherited. `size_of`, pinned by a new test:
Option<f32> 8 Peak 24 -> 16 (no IM) -> 8 (f32 m/z)
Ms2Scan 104 -> 80
Resident MS2, from the engine's own `mem: ms2 scans` report:
fixture (480 scans, 30,391 peaks) 779,304 B -> 281,528 B -63.9%
AIF_01 (465,806 scans, 41,293,465 peaks) 0.968 GiB -> 0.342 GiB -64.6%
Both memlog parts solve to the same peak count either side, which is the
accounting cross-check. Scaled to the analysts' 171.6 M-point HYE Astral run:
3.84 GiB -> 1.28 GiB. No wall-clock claim: the paired search-seed timings on
this contended desktop spread 7.0-23.3 s baseline against 9.6-20.0 s, which
swamps any cache effect.
Output equality: proved, not asserted. `f32 -> f64` is exact, so every
`peak.mz as f64` yields the bit pattern the f64 field held, no ppm predicate
changes and no float reduction is reordered. Checked two ways against a
`perf/stack` binary built and copied aside before the edit:
- `ci/smoke.sh` end to end under both binaries, SMOKE_OK from each, and all 189
produced parquet/TSV/mzML files byte-identical by sha256 -- the ungrouped run,
the determinism re-run, the NaN-RT run, both `window_groups: 3,
calibration: per_group` grouped runs and the two-run experiment;
- real data: `search-seed` over `LFQ_Orbitrap_AIF_Ecoli_01` (465,806 scans, the
1.7M-candidate DIA-NN library, 144,565 PSMs, 6,148 confident) gave a
byte-identical `seed_psms.parquet` and a byte-identical `masscal.json`, which
covers the ppm-deviation loop this change edits.
Tests: `peak_is_two_f32_and_nothing_else` and `f32_mz_widens_to_f64_exactly`
pin the layout and the exactness argument; `ms2_peak_mz_is_the_artifact_f32_...`
pins the loader's old behaviour by asserting the new expression against the old
one on f32-awkward values. `tests/pipeline.rs` shared-buffer byte-identity tests
unchanged and passing.
`docs/24_config_reference.md` is regenerated because three env-read line numbers
in `extract.rs` moved. Its "9 environment read(s)" -> "3" count line was already
stale on `perf/stack` (verified by running `--check` against the pristine base);
the generator prints `len()` of a list it then deduplicates.
Not done, and deliberately: the flat CSR peak buffer. At 8-byte peaks a 300-peak
scan block is 2.4 KB, the per-scan `Vec` headers are 37 MB of a 465,806-scan run,
and turning `Ms2Scan` into a non-owning view would touch every stage signature
for about 3% of what this commit already removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the integration branch for three changes already up separately, plus one that builds on them. If you merge #112, #113 and #114 individually, rebase this and only the peak layout remains; if you merge this, those three can be closed.
Contains:
Peaknarrowed from 24 bytes to 8The peak layout
A decoded peak carried 24 bytes for 8 bytes of data.
PeakMs2Scanion_mobilityf32m/z and noidion_mobilityis anOption<f32>, which is 8 bytes because an f32 has no niche, and it is written at three sites and read at none.mzwas anf64widened at load from thef32the spectra artifact stores, so it provably never held more than the f32;Ms1Scan.mzis alreadyVec<f32>with that same justification in its own doc comment. The 22 read sites widen at the point of use.Measured from the engine's own memory report rather than by arithmetic, on
LFQ_Orbitrap_AIF_Ecoli_01with 465,806 scans and 41,293,465 MS2 points: 0.968 GiB to 0.342, down 64.6%. On the smoke fixture, down 63.9%. Scaled to the HYE Astral shape the analysts measured, 3.84 GiB becomes 1.28.That compounds with the first change in this branch. Before today a grouped run held one copy of the spectra per band in flight, so 48 bands at 3.84 GiB was 184 GB; it is now one copy at 1.28.
One correction to the survey that found this:
Ms2Scan.idis not read at zero sites. It is read at two, a debug-only fingerprint and one test assertion, neither of which feeds a computation or an output column, so it goes, but the claim as written was wrong.No wall-clock claim is made. The paired seed timings on the author's contended desktop spread 7.0-23.3 seconds against 9.6-20.0, which is machine noise rather than a cache effect.
Equality
Byte-identical, proved twice. Both binaries built and copied aside, then
ci/smoke.shrun against each: 189 of 189 produced parquet, TSV and mzML files identical by sha256, covering the ungrouped run, the determinism re-run, the NaN retention-time run, both groupedper_groupruns and the two-run experiment. On real data,search-seedover the converted AIF file against the 1.7M-candidate DIA-NN library gives 144,565 PSMs and 6,148 confident in both, withseed_psms.parquetand itsmasscal.jsonbyte-identical; that sidecar carries the ppm deviations from the loop the narrowing touches, so the path is directly covered.cargo fmt --check,clippy -D warnings, 408 tests,ci/smoke.sh→ SMOKE_OK.🤖 Generated with Claude Code