fix(test): the unmapped-reason test re-implemented the logic it was testing - #169
Open
BenjaminDEMAILLE wants to merge 5 commits into
Open
fix(test): the unmapped-reason test re-implemented the logic it was testing#169BenjaminDEMAILLE wants to merge 5 commits into
BenjaminDEMAILLE wants to merge 5 commits into
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>
The test at `read_align.rs` built its own `HashMap<&str, i32>` and re-implemented the `has_mismatch` / `has_other` decision inside the test body, then asserted on its own copy. It would have passed unchanged if the shipped code had been deleted. It now drives `FilterReasons`, which is the same set of counters the aligner uses, so a change to that decision fails the test. `FilterReasons` replaces the `HashMap<&str, i32>` the filter loop used: a fixed `[u32; 9]` indexed by constant. That was a heap allocation per read and a string hash per filtered transcript, for something whose only consumers ask whether a count is non-zero (two debug logs and the `unmapped_reason` decision), so nothing here can reach the output except through that decision. Its `Debug` prints only the non-zero reasons, in a fixed order, which is more readable than the map's arbitrary one. `SpliceJunctionDb` moves to `FxHashMap` in the same spirit, matching what `cluster_seeds` already does for its integer-keyed maps. Only `get` and `insert` are called on it and it is never iterated. Honest about the performance: **neither change is measurable end to end.** Six interleaved rounds on 2M reads gave medians 20.70 s against 20.48 s with the direction mixed, which is inside the run-to-run spread. They are here because they remove work and because the test was not testing anything, not because they show up on a clock. 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>
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.
A test that was not testing the shipped code, and the per-read allocation it was hiding.
The test
read_align.rshad this inside the test body:It rebuilds the decision rather than calling it. The assertions below it are fine, but they check a copy of the logic that lives in the test, so the test would pass unchanged if the production code were deleted. It is asserting that the test author can write an
if.It now drives
FilterReasons, the same counters the aligner uses, so changing that decision fails the test.What
FilterReasonsisMaking the test call the real thing meant giving the real thing a name. The filter loop kept its counts in a
HashMap<&str, i32>built per read and incremented per filtered transcript: a heap allocation on every read and a string hash on every increment, for something whose only consumers are two debug logs and one boolean decision.It is now a
[u32; 9]indexed by constant. The consumers ask only whether a count is non-zero, so nothing here can reach the output except through theunmapped_reasondecision, which is unchanged and now under test.Debugprints the non-zero reasons in a fixed order, which reads better than the map's arbitrary one.SpliceJunctionDbmoves toFxHashMapin the same spirit — it is queried per candidate junction while stitching, andcluster_seedsalready made this choice for its integer-keyed maps. Onlygetandinsertare ever called on it and it is never iterated, so the hasher cannot reach the output.Performance: no
Neither change is measurable end to end, and I would rather say so than dress this up as a perf PR. Six interleaved rounds on 2M reads at 16 threads:
Direction mixed across the six, inside the run-to-run spread, and the machine sat at 72-85% idle rather than the 88% the bench script gates on. Call it unmeasured.
They are here because the test was not testing anything and because both changes remove work that has no reason to exist, not because of a number.
For scale on why one allocation per read is not measurable: the align path does about 1 000 heap allocations per read (measured in #168). This removes one of them.
Verification
Output-neutral: SAM byte-identical on 200 000 real reads (
ERR12389696, yeast) against the parent commit, whole file including header.Gate: 561 lib + 26 integration tests,
cargo clippy --all-targets -- -D warnings,cargo fmt --check, all green.