Fix one-gap adapter search offset (#728) and speed up matchWithOneInsertion

[dougnukem]

Fixes #728.

trimBySequence's one-gap search compared the adapter with the start of the read at every position, instead of at the position being tested. So it missed adapters with a one-base indel, and it trimmed the tail of reads that only start like the adapter (~1.8% of reads on a public WGBS run). Details and a 5-read reproduction are in #728.

Changes

1. fix: pass rdata + pos to both Matcher::matchWithOneInsertion calls in AdapterTrimmer::trimBySequence, the same offset the exact-match loop already uses.
2. perf: early-exit Matcher::matchWithOneInsertion.
   • Why: with (1) the search does the comparisons it was meant to do, and it's already the largest function in the worker threads (33% of worker CPU in a perf profile of SRR10007843).
   • Old version: built the full prefix and suffix mismatch arrays on every call, including an O(length) sentinel fill.
   • New version: scans the prefix forward and the suffix backward, stops each scan once it can't be part of a match within diffLimit, then checks only the splits both scans reached. On unrelated sequence both stop within a few bases.
   • Same results: the previous implementation is kept as matchWithOneInsertionReference. It's used for cmplen > 512 and by the new Matcher::test.
3. tests:
   • AdapterTrimmer::test gains a TruSeq adapter at position 40 with a one-base insertion and a one-base deletion (both fail on master), and a read that starts like the adapter (master cuts it from 81 to 72 bp).
   • Matcher::test compares the new and reference matchWithOneInsertion on 200k random, near-match and exact cases.

Verification

• fastp test passes. Each new test fails without the change it covers.
• Matcher::test catches deliberate off-by-one mutations of the new function (4 of 4 tried).
• Full-size output of "fix only" and "fix + early-exit" is byte-identical on all three runs below. The early-exit change is a pure speed change.

Performance

Complete public runs, fastp -w 16 (PE with --detect_adapter_for_pe), GCE n2d-highmem-48, inputs in page cache. Mean of 2 runs in alternating build order.

run	1.3.7 wall / CPU	fix only	fix + early-exit (this PR)
SRR10007843 PE 150	59.5 s / 827 CPU-s	79.9 s (+34%) / 1171 (+42%)	67.4 s (+13%) / 954 (+15%)
ERR10308506 WGBS PE	69.6 s / 578 CPU-s	72.4 s (+4%) / 647 (+12%)	68.1 s (−2%) / 561 (−3%)
ERR10669429 SE 75	38.8 s / 333 CPU-s	40.7 s (+5%) / 427 (+28%)	39.7 s (+2%) / 357 (+7%)

A separate benchmark on GitHub-hosted 4-CPU runners, which projects 300K–500K-pair subsets to a 50M-read run (synthetic PE/SE 150 bp and SRR891268 ATAC PE 50, -w 1 and -w 4), gave:

build	projected wall vs master	projected CPU vs master
fix only	+11% to +45%	+12% to +30%
fix + early-exit	−9% to +12%	−9% to +8%

Peak RSS is unchanged.

So the fix makes trimming correct at roughly the old cost. It isn't a speedup, since the search now does the work it was meant to do. matchWithOneInsertion is still the largest worker function after this change. A bit-parallel or SIMD version of the one-gap search could take it further, but would change results in edge cases, so it's left out of this PR.

Output changes users will see

• More adapter-trimmed reads on libraries with indel-bearing adapters (+0.5% to +0.6% on the RNA-seq runs above).
• Fewer false trims on libraries whose reads often start like the adapter. The WGBS run above trims 15% fewer reads, because fastp detects a T-run-prefixed adapter there and bisulfite reads are T-rich.

🤖 Generated with Claude Code

[dougnukem]

Hardware-counter data on where the fix's extra CPU goes, in case it helps review.

Setup. GCE c4-standard-96 with hyperthreading off (48 physical cores, Intel Emerald Rapids) and the PMU enabled. Complete SRR10007843 (31M pairs, 150 bp, --detect_adapter_for_pe), -w 16, one run per build.

• Before is my fork's build of upstream master plus unrelated changes (backpressure scaling as in fix: scale reader backpressure with worker thread count (#721) #723, an adapter-detection index, pull-based work claiming). Its per-read code is unchanged and its profile matches upstream 8a2397b (IPC 2.67 vs 2.65).
• After is that build with ffd38a1 and 0b615b5 applied.
• I did not run counters for the fix without the early exit.

	before (old search)	after (this PR)
instructions	9.06T	7.51T (−17%)
cycles	3.42T	3.77T (+10%)
IPC	2.65	1.99
branch-miss rate	2.83%	5.24%
top-down: retiring / bad speculation	39.9% / 20.5%	31.1% / 31.0%
matchWithOneInsertion: share of cycles / IPC / branch-misses per 1k instr	28% / 4.4 / 1.6	36% / 2.0 / 11.0

Reading. The PR executes fewer instructions than the old search but takes about 10% more cycles. My interpretation, not something I tested: the old loop compared the same bytes at every position, so its branches were almost always predicted; the corrected search has data-dependent exits that mispredict. That fits the +13–15% wall/CPU in the table above. It also means instruction counts are a poor proxy here: cycles or CPU time are what to compare.

If someone wants to bring the remaining cost down, the mispredictions are the target rather than instruction count. In the PR build, matchWithOneInsertion and the SIMD CountMismatchesBoundedImpl together account for about two thirds of all branch misses (43% and 25%). I haven't tried a branch-free variant, so I can't say what it would gain.

Raw counter files and the profiling scripts are in dougnukem#9 (benchmark/results/pmu-2026-09/) if you want to check the numbers.

🤖 Generated with Claude Code