Skip to content

Add reproducible FASTQ reader performance benchmarks - #22

Merged
rob-p merged 5 commits into
mainfrom
decoded-buffer-recycling
Aug 4, 2026
Merged

Add reproducible FASTQ reader performance benchmarks#22
rob-p merged 5 commits into
mainfrom
decoded-buffer-recycling

Conversation

@rob-p

@rob-p rob-p commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds a reproducible performance and correctness harness for
programmatic decoding through ordinary Read, IndexedReader, and actual
paraseq FASTQ consumption. It also documents why automatic cross-thread
DecoderReader buffer recycling was evaluated and rejected.

There are no decoder-core or CLI runtime changes in this PR.

What is included

  • Deterministic, exact-length FASTQ generation with a paraseq correctness
    preflight.
  • Single-member, sparse multi-member, dense multi-member, and BGZF archive
    shapes.
  • Ordinary, indexed, and paraseq reader modes with configurable worker counts,
    consumer buffer sizes, delays, early stopping, and repeated decodes.
  • An alternating A/B runner that pins both revisions to the same CPU set and
    records wall, user, system, throughput, and peak-RSS measurements.
  • Paired reporting that computes candidate-versus-main deltas within each
    repetition before aggregation, reducing host-state and NUMA bias.
  • Benchmarking instructions and a full design assessment of decoded-buffer
    recycling.

Recycling assessment

The strongest prototype substantially reduced allocation volume for a 128 MiB
single-member decode:

revision allocated bytes allocation blocks bytes at global heap peak
exact main 145,807,326 83 23,119,112
prototype 53,533,488 67 18,925,675

It nevertheless failed the default-path throughput gate. In fresh, isolated
builds over nine paired, 30-archive observations, the final automatic prototype
was 2.4% faster for 8 KiB reads but 4.3% slower for 1 MiB reads. Earlier
unrestricted recycling also made one-worker dense-member paraseq about 6%
slower. Consumer- and member-shape admission restored dense-member and BGZF
parity, but could not keep the ordinary bulk path structurally identical to
main.

The runtime implementation was therefore removed from this PR. The exact
prototype and its implementation documentation are archived on
decoded-buffer-recycling-experiment
for reproducibility.

Validation

  • cargo test --locked --workspace --all-features
  • cargo clippy --locked --workspace --all-targets --all-features -- -D warnings
  • cargo doc --locked --workspace --all-features --no-deps
  • shell syntax and paired-report unit tests
  • ordinary and paraseq smoke matrices over all four archive shapes at 1, 4,
    and 16 requested workers
  • CI on Linux, macOS, Windows, and AArch64, plus MSRV, index interoperability,
    and benchmark-tool jobs

This is the clean follow-up to the decoded-buffer recycling idea split from #5.

@rob-p rob-p mentioned this pull request Aug 4, 2026
@rob-p
rob-p force-pushed the decoded-buffer-recycling branch from f0919b1 to 8de2046 Compare August 4, 2026 03:31
@rob-p rob-p changed the title Design bounded decoded-buffer recycling Recycle positional reader output buffers Aug 4, 2026
@rob-p
rob-p marked this pull request as ready for review August 4, 2026 03:32
@BenjaminDEMAILLE

Copy link
Copy Markdown
Contributor

Reviewed and built locally on this branch: cargo fmt --check clean, cargo clippy --workspace --all-targets --all-features clean, full suite green apart from final_reader_handoff_reports_consumer_backpressure.

That failure is worth a note because it lands exactly where this PR works. Since the pool changes the reader handoff, a failure there is where a real defect from this change would surface, so I measured it rather than assuming: 1/10 failures on main, 2/10 here. Ten runs cannot distinguish those, so I read it as the same pre-existing flake and not something this PR introduced. I opened #24 to fix it properly: the test asserts spawned_workers <= 1, but a worker parked in the bounded handoff owns a verified unread chunk and cannot retire without discarding validated output. It should assert capped admission instead.

On the change itself, the ownership design reads correctly to me:

  • accounting is charged before publication and refunded on the entry-count rejection path, so the two counters cannot drift;
  • take decrements before deciding whether the stolen buffer is large enough, so a rejected steal does not leak accounting;
  • a capacity above the largest ceiling cannot reach the unwrap_or(len - 1) fallback in recycle, because maximum_capacity is rejected earlier and the top ceiling is at least that large;
  • emit staying unpooled means marker and specialized paths cannot return an allocation the pool would then hand to a path expecting a different size class.

Two things I checked specifically and found fine rather than problematic:

take can return a zero-capacity Vec when the pool is empty, and every emit_reusable caller reserves against decoded_chunk_size before its next inflate, so that path is safe.

A pooled buffer grown past maximum_capacity by a caller is simply rejected on recycle, which costs a miss and not correctness.

One question rather than an objection. for_reader sets maximum_bytes = maximum_capacity, so the byte ceiling equals a single largest-class buffer while maximum_entries allows up to four. With minimum_capacity = chunk/2 the two bounds agree at two chunks, but they are derived independently, and a future change to either constant could make the entry bound unreachable without anything failing loudly. A debug assertion tying them together, or deriving one from the other, would keep that intent from decaying silently.

@rob-p
rob-p force-pushed the decoded-buffer-recycling branch from 67ea2f9 to cb5cb38 Compare August 4, 2026 15:05
@rob-p rob-p changed the title Recycle positional reader output buffers Add reproducible FASTQ reader performance benchmarks Aug 4, 2026
@rob-p
rob-p merged commit bc76bbf into main Aug 4, 2026
16 checks passed
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