Conversation
Counter<K> sits on the hot per-record path — every alignment/isize/rna record calls .count(). std's default SipHash is cryptographically strong but slow, and the counter keys are small non-adversarial integers, so fxhash (rustc-hash, already a dependency — error.rs uses it) is the right trade. This is a backing-store swap only; output is byte-identical (all exact-output integration tests pass unchanged). Step 1 of the Counter redesign; a dense+overflow hybrid follows once profiling sizes its marginal gain.
A sweep for std SipHash HashMap/HashSet on high-volume paths (same motivation as the Counter fxhash swap) turned up two worth changing: - rna: `ignored_ref_ids` is probed per record in `accept()` to divert ignore-list reads -- the one genuinely per-record std HashSet. - sequence_dict: `name_to_index` is setup-only today, but it's a foundational contig-name lookup that could land on a hot path later, so default it to a non-cryptographic hasher now. Everything else on std SipHash is cold (per-contig transitions, finish()-scoped scans, or one-time annotation parsing) and left as-is.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughInternal hash-based collections across the codebase are switched from Rust's standard Changes
Related Issues: None referenced. Related PRs: None referenced. Suggested labels: performance, refactor Suggested reviewers: None specified. Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The GFF/GTF parsers and the shared transcript->gene grouping build HashMap<String, ...> keyed by feature/transcript/gene ids -- one insert per annotation line, millions for a full GENCODE. It's a one-time startup cost (not per-record), but swapping std SipHash for fxhash shaves a little off gene-model load for free. Output is byte-identical: the maps are drained into sorted output, so iteration order never mattered (std's per-run SipHash randomization already guaranteed that).
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
What
Swaps std's default SipHash for fxhash (
rustc-hash, already a dependency) wherever hash maps/sets sit on high-volume paths:Counter<K>(src/counter.rs) — the frequency counter behind the isize/alignment/rna histograms;.count()runs on the per-record hot path.rna::ignored_ref_ids— aHashSet<usize>probed on every record inaccept().sequence_dict::name_to_index— setup-only today, but a foundational contig-name lookup that could land on a hot path later.HashMap<String, …>keyed by feature/transcript/gene id, one insert per annotation line (millions for a full GENCODE). One-time at startup, not per-record, but a free shave off gene-model load.Output is byte-identical; all 771 tests pass.
Why (measured, not guessed)
std SipHash is DoS-resistant but ~4–5× slower per op than fxhash, and none of these keys are attacker-controlled. Microbenchmarked at the real HG03953.2× record counts:
|TLEN|(22.7M)For the
Counterswap that's ~0.7–1.4% of alignment/isize wall-clock (more at higher thread counts, where decode is offloaded and the denominator shrinks) — byte-identical.The gene_model maps are drained into sorted output, so iteration order never mattered (std's per-run SipHash randomization already guaranteed that); fxhash is a safe drop-in.
What was deliberately NOT done
A dense-array + overflow
Counter(array index instead of any hash) was prototyped and profiled. It's a further 1.4–3.3× faster percount(), but worth only ~0.1–0.2% of runtime: both commands are decode-bound (libdeflate is 46–76% of samples; evenmulti --threads 3amortized over three counter-using tools keepsCounter::countat 0.2%). It failed the "only optimize hot paths" bar and would have added aCounterKeytrait + dense/overflow machinery for no measurable gain, so it was dropped.Sweep coverage
Every other std
HashMap/HashSetinsrc/is cold (per-contig transitions,finish()-scoped scans, or one-time init) and left on the default.