Skip to content

[bench 1/4] reader backpressure scales with threads (#723) - #5

Draft
dougnukem wants to merge 3 commits into
masterfrom
bench/1-backpressure
Draft

dougnukem wants to merge 3 commits into
masterfrom
bench/1-backpressure

Conversation

@dougnukem

Copy link
Copy Markdown
Owner

Layer 1: the OpenGene#721 deadlock fix (upstream OpenGene#723).

Cumulative: includes every layer below it, so the benchmark comment compares the whole stack up to here against master (upstream + CI only). The per-layer effect is the difference from the previous PR's comment.

🤖 Generated with Claude Code

Each worker owns one input list, and SingleProducerSingleConsumerList only
lets a consumer take an item once another item has been produced behind it
(or the producer has finished). Reader backpressure used a fixed
PACK_IN_MEM_LIMIT (32) that is smaller than the worker count on machines
with more than 32 cores. With 33+ workers the readers stop after 33 packs,
before any worker list holds two items, so no pack is ever consumed and all
reader and worker threads wait on the backpressure condition variable
forever. The writer-backlog check has the same shape: the writer drains
worker lists round-robin and waits on a list until its worker produces a
second output, so a fixed limit below the worker count can also block the
readers permanently.

Allow at least two in-flight packs per worker in both the reader/processor
and reader/writer backpressure checks (max(PACK_IN_MEM_LIMIT, 2 * threads)),
for PE, interleaved PE and SE readers. The lock-free list semantics are
unchanged, so this does not reintroduce OpenGene#695.
The reader/writer backlog check still gated on the fixed
PACK_IN_MEM_LIMIT * PACK_SIZE period instead of the new
mPackInMemLimit, so it fired far more often than the (now larger)
buffer actually needs at high thread counts. Not a correctness bug —
the comparisons inside already used mPackInMemLimit — just an
inconsistency worth cleaning up alongside it.

Also removed a stray extra blank line left in common.h.
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

fastp benchmark

✅ No regressions over 10%.

base → head, median of 3 interleaved runs on one 4-CPU runner. 🔴 = worse by more than 10%, 🟢 = better by more than 5%, in both cases with no overlap between the base and head runs; unmarked changes are within runner noise. Wall time, CPU and peak RSS are gated; stage timings are informational.

dataset -w wall time CPU (user+sys) peak RSS
synthetic_pe 1 4.49 → 4.61 s (+2.7%) 7.30 → 7.45 s (+2.0%) 1238.81 → 1238.52 MB (-0.0%)
synthetic_pe 4 3.11 → 3.18 s (+2.3%) 8.72 → 8.93 s (+2.4%) 1232.69 → 1232.57 MB (-0.0%)
synthetic_se 1 2.22 → 2.24 s (+1.1%) 3.22 → 3.29 s (+2.3%) 1199.05 → 1199.01 MB (-0.0%)
synthetic_se 4 1.72 → 1.75 s (+1.4%) 4.39 → 4.51 s (+2.7%) 1194.37 → 1194.49 MB (+0.0%)
atac_hiseq_pe 1 4.42 → 4.49 s (+1.5%) 7.40 → 7.46 s (+0.8%) 1228.26 → 1226.79 MB (-0.1%)
atac_hiseq_pe 4 2.89 → 2.95 s (+2.0%) 8.52 → 8.76 s (+2.8%) 1217.88 → 1217.80 MB (-0.0%)
stage timings
dataset -w adapter detection processing
synthetic_pe 1 0.93 → 0.92 s (-1.0%) 3.43 → 3.57 s (+4.2%)
synthetic_pe 4 0.92 → 0.92 s (-0.5%) 2.09 → 2.17 s (+3.5%)
synthetic_se 1 0.56 → 0.56 s (-0.0%) 1.59 → 1.60 s (+0.4%)
synthetic_se 4 0.56 → 0.56 s (-0.2%) 1.11 → 1.13 s (+2.3%)
atac_hiseq_pe 1 0.70 → 0.71 s (+1.2%) 3.59 → 3.66 s (+1.8%)
atac_hiseq_pe 4 0.70 → 0.70 s (-0.9%) 2.12 → 2.18 s (+2.8%)

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.

2 participants