Skip to content

perf(io): encode SAM/BAM records on the align workers (stacked on #276) - #278

Open
BenjaminDEMAILLE wants to merge 1 commit into
perf/parallel-bgzf-writerfrom
perf/parallel-record-encoding
Open

BenjaminDEMAILLE wants to merge 1 commit into
perf/parallel-bgzf-writerfrom
perf/parallel-record-encoding

Conversation

@BenjaminDEMAILLE

Copy link
Copy Markdown
Contributor

Stacked on #276 (base perf/parallel-bgzf-writer). Refs #223: the follow-up that moves record encoding off the single writer thread.

  • New src/io/encode.rs: RecordEncoder produces SAM/BAM bytes through the same noodles writers the output uses (NoQS stripping, CIGAR check included).
  • Each batch's records (main, WithinBAM supplementary, TranscriptomeSAM) are encoded in parallel on the align workers; writers gain write_encoded.
  • Sorted BAM: SortBuffer stores encoded records and stable-sorts an index on (ref id, start), unmapped last, same order as before.
  • Gated: the old writer thread is effectively a free extra core, so pre-encoding costs 0.5 to 1.3s at 1 to 8 threads. It turns on when runThreadN + spare > available cores (spare 4 for BAM, 6 for SAM). BySJout always keeps the old path. RUSTAR_PRE_ENCODE=1|0 forces it.

Wall time (s), 16-core Mac, 2M SE 100 bp reads, median of 3:

threads None BAM main BAM #276 BAM this PR (auto) SAM main SAM forced on
1 12.55 15.19 13.79 14.04 14.11 14.37
8 2.87 3.07 2.99 3.00 3.03 3.19
12 2.33 3.36 2.51 2.52 2.87 2.48
16 2.26 3.94 2.69 2.42 3.19 2.41

Identity (cmp vs main, forced on): SAM, BAM unsorted/sorted, --outStd, NoQS, BySJout, TranscriptomeSAM, GeneCounts, unmapped Fastx, PairedKeepInputOrder, two-pass, PE chimeric WithinBAM, solo: all identical. tests/pre_encode_parity.rs covers 14 configurations on/off.

Conflicts: 3-way merge simulation of src/lib.rs against #222 and #261 gives 0 conflicts. Nearest hunks are the solo batch_size lines; if they conflict, keep both lines. #261 note: AlignmentBatchResults grows by 48 bytes, so batches get slightly smaller (no output change).

Pre-existing, not fixed here: SE --chimOutType WithinBAM fails on main ("read length-sequence length mismatch"); BufferedSamRecords::new() reserves 10,000 records per read.

Tests: fmt, clippy 0 warnings, cargo test 639 passed.

🤖 Generated with Claude Code

…saturated

RecordEncoder serializes records to SAM/BAM bytes inside the rayon align
stage; the writer thread only appends pre-encoded bytes. Sorted BAM
buffers encoded records and sorts an index. Enabled when runThreadN plus
a spare margin (4 BAM, 6 SAM) exceeds available cores, since the writer
thread is otherwise a free extra core; RUSTAR_PRE_ENCODE=0/1 overrides.
BySJout keeps the old path. Output is byte-identical (parity test over 14
configurations).

Refs #223

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant