Batch PRs #167 and #185 - #215
Merged
Merged
Conversation
`detect_splice_motif` and `find_best_junction_position` were the two largest self-time entries in a sampled 2M-read run, together about 45% of the on-CPU samples. The scan moves the junction one base per iteration, so the four bases that decide the motif (the intron's first two and last two) overlap the previous position in two of four places. Carrying them in a `MotifWindow` and sliding turns four genome reads per position into two. Whether the motif branch runs at all is decided once before the loop rather than per iteration, since `del` does not change across the scan. An out-of-range position becomes a sentinel that no motif arm matches, which is what `get_base` returning `None` already meant. Output-neutral: SAM byte-identical to the parent commit on 200k real reads. Interleaved A/B at 16 threads on 2M reads, machine at 87-95% CPU idle: 22.94s median before, 22.29s after, the same direction in all four rounds. About 2.8%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four-base motif decision compiled to a chain of comparisons whose
outcome is data-dependent and close to unpredictable, and the junction
scan pays it at every position. A 256-entry table indexed by the four
bases packed two bits each replaces it with one load.
`examples/jrbench.rs` measures the scan in isolation: 8.5 ns per
iteration before, 5.2 ns after, a 38% cut, with identical results.
That example exists because the earlier attempts on this path were guesses
about whether the loop was memory-bound, and two of them were wrong. It
answers the question by construction, holding the iteration count and the
instruction mix fixed and changing only how far the acceptor sits from the
donor:
del ns/iter
64 8.42
1024 8.47
16384 8.52
262144 8.41
4194304 8.62
67108864 8.56
Flat from 64 bases to 67 million, so the acceptor distance costs nothing
and the loop is not stalled on that stream. Which is why skipping the
acceptor read behind a donor-pair test measured *slower*: it traded a
prefetchable access for an unpredictable branch. The branch was the cost
all along, and this removes it.
`the_motif_table_agrees_with_the_original_match_on_every_input` checks
the table against the match it replaces over every combination of
`A`/`C`/`G`/`T`, `N`, the chromosome-boundary byte and the out-of-range
sentinel, so the inputs that no match arm covered are covered explicitly.
Output-neutral: SAM byte-identical on 200k real reads.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cluster_seeds built a fresh FxHashMap per read. The pre-sizing to anchor_indices.len() * 2 was only a floor: merging two windows re-keys every bin in the merged span, so a read with wide windows inserts far more entries than it has anchors and the map rehashes. Sampling a human 10x run put hashbrown::reserve_rehash at 2.1% of on-CPU time, all of it here. The map is now kept per thread and cleared per read, so its capacity settles at what the workload needs and the growth is paid once per thread. It is taken out of the thread-local rather than borrowed across the body, so a nested call would get a fresh map instead of panicking on an active borrow. Output-neutral: matrix.mtx, barcodes.tsv, features.tsv and SJ.out.tab are byte-identical before and after on 20 M read pairs of 10x pbmc_1k_v3 against GRCh38, compared decompressed. Interleaved A/B, 8 pairs: new faster in 8 of 8, median -7.0%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The per-window Vec<WindowAlignment> was cloned into the SeedCluster while `windows` was dropped one statement later, so every read paid a full copy per window to throw the original away. std::mem::take moves it instead. Output-neutral, verified by an empty diff on 20 M read pairs of 10x pbmc_1k_v3 against GRCh38. 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
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.
Batch merge of the two output-neutral performance PRs from @BenjaminDEMAILLE, plus a fix to the benchmark harness found while measuring them. His commits are preserved with original authorship; the batch is split by commit so it reads PR by PR.
Closes #167
Closes #185
What's in it
289cfbe,0c0ec81,961366fba152cb,91a848acluster_seedsreuses its window-bin map across reads on the same thread instead of allocating and rehashing per read, and moves each window's alignments into its cluster instead of cloning them.#185's two commits were taken as their own increment. The PR displays the CellRanger stack beneath it, but the change itself touches only
src/align/stitch.rsand applies ontomainwithout any of it.Changes made while merging
test/bench_ab.shcould not run on Linuxcpu_idleusedtop -l 2 -n 0, which is macOS syntax; on Linux we can dotopexits non-zero,set -o pipefail, and the script prints its header and exits 0 with no error. Separately, each round runs from a private temp directory butGENOME_DIR/READSwere used as given, so relative paths stopped resolving after thecd— same silent-abort symptom. Both fixed:cpu_idlebranches onunameand reads two/proc/statsamples a second apart on Linux, and both paths are absolutised at startup.#167's CHANGELOG entry cited a benchmark the branch cannot produce — "the scan itself goes from 8.5 ns to 5.2 ns per iteration, measured by
examples/jrbench.rs".jrbenchhas no before/after mode; it measures the current build across a range ofdelvalues, so it can produce one of those numbers and not the pair. Replaced with the wall-clock A/B below and a pointer totest/bench_ab.sh, which anyone can run.Some CHANGELOG organisation cleanup
Validation
Output-neutral Records compared with headers excluded, since the
@PG CL:line necessarily differs by output prefix:main--runThreadN 1vs--runThreadN 4That last row is the one #185 needs. It introduces a
thread_local!map reused across reads, so results could in principle depend on which reads a thread happened to process first. Checked structurally as well:win_binis only everget,insertandentry().or_insert()— it is never iterated, so its capacity cannot reach the output. The thread-count comparison confirms that empirically.Against STAR 2.7.11b, unchanged from the recorded baselines:
Speed. Interleaved A/B, 100k yeast reads,
--runThreadN 4,--outSAMtype Noneso the measurement is alignment work rather than the writer:−12.9% median, 5 of 5 rounds kept, every round in the same direction. The spread within each column (0.34 s / 0.47 s) is well inside the gap.
The two PRs claimed ~2.8% and ~7% separately on a different workload (2M reads, 16 threads); those compound to roughly this, so the scale is consistent rather than surprising.
Reproduce with the dataset from CONTRIBUTING: