Skip to content

bench: per-wave shard/read diagnostics - #159

Merged
nclack merged 1 commit into
mainfrom
probe/array-starve
Jun 15, 2026
Merged

nclack merged 1 commit into
mainfrom
probe/array-starve

Conversation

@nclack

@nclack nclack commented Jun 15, 2026

Copy link
Copy Markdown
Owner

Per-wave dispatch instrumentation, built to investigate #154 (does read/decode
parallelism track array/shard count or the io pool?). Generally useful for any
read-width / wave-shape diagnosis, so proposing it for main.

What it adds

  • damacy_stats counters (emitted in bench results.json): wave_reads_sum,
    wave_distinct_shards_sum, and wave_stop_{drained,host,chunks,dev}. Means are
    the sums over waves_emitted — shards/wave is the io width the coalescer's
    round-robin actually achieves; the stop_* split says whether waves cap on a
    budget (more work waiting) or drain (no more work).
  • wave_desc.stop_reason + enum wave_stop_reason, set at each break in
    wave_dispatcher_reserve.
  • DAMACY_TRACE_WAVES=<file> (opt-in): one line per reserved wave —
    batch_id render_job_idx n_reads distinct_shards n_chunks input_bytes stop_reason.
    Mirrors the existing DAMACY_TRACE_READS pattern.
  • bench/sweep.py: reads/wave, shards/wave, stop d/h/c/v columns.
  • bench/scenarios/array-starve.json: an n_zarrs sweep scenario (large
    shards) for sweep.py --param dataset.n_zarrs.

Always-on cost is one O(n) pass plus a small distinct-shard scan per wave
(n_reads is post-coalesce, typically 100–250); negligible vs io/decode. The trace
is opt-in.

What it found (full writeup on #154)

Using these counters, io throughput scales cleanly with shards/wave — 6→1.09,
37→2.37, 236→4.32 GB/s — confirming a real read-width effect on few-shard sets.
But it's mild: damacy stays io-bound and beats tensorstore on every measured
dataset, including a 1-array one
. The catastrophic losses that motivated #154
were decode amplification, not array count. So #154 wants re-scoping to
"io-pool under-utilization on few-shard datasets" rather than a competitiveness bug.

Not included

bench/tensorstore_bench.py (used for the comparison) lives on its own branch
bench-tensorstore-comparison (#153) and is intentionally not duplicated here.

ctest 33/33 on L40.

@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 51.92308% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.68%. Comparing base (cca3b09) to head (6bf8e81).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/wave/wave_input.c 60.60% 11 Missing and 2 partials ⚠️
bench/main.c 0.00% 12 Missing ⚠️

❌ Your patch check has failed because the patch coverage (51.92%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   57.78%   57.68%   -0.11%     
==========================================
  Files          64       64              
  Lines       10046    10104      +58     
  Branches     1748     1754       +6     
==========================================
+ Hits         5805     5828      +23     
- Misses       3494     3523      +29     
- Partials      747      753       +6     
Flag Coverage Δ
unittests 57.68% <51.92%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/render_job/render_job.c 84.51% <100.00%> (+0.41%) ⬆️
bench/main.c 0.00% <0.00%> (ø)
src/wave/wave_input.c 72.64% <60.60%> (-5.45%) ⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nclack
nclack merged commit 7d860ac into main Jun 15, 2026
6 checks passed
@nclack
nclack deleted the probe/array-starve branch June 15, 2026 04:45
github-actions Bot added a commit that referenced this pull request Jun 15, 2026
Per-wave dispatch instrumentation, built to investigate #154 (does
read/decode
parallelism track array/shard count or the io pool?). Generally useful
for any
read-width / wave-shape diagnosis, so proposing it for `main`.

## What it adds

- **`damacy_stats` counters** (emitted in bench `results.json`):
`wave_reads_sum`,
`wave_distinct_shards_sum`, and `wave_stop_{drained,host,chunks,dev}`.
Means are
the sums over `waves_emitted` — `shards/wave` is the io width the
coalescer's
round-robin actually achieves; the `stop_*` split says whether waves cap
on a
  budget (more work waiting) or drain (no more work).
- **`wave_desc.stop_reason`** + `enum wave_stop_reason`, set at each
break in
  `wave_dispatcher_reserve`.
- **`DAMACY_TRACE_WAVES=<file>`** (opt-in): one line per reserved wave —
`batch_id render_job_idx n_reads distinct_shards n_chunks input_bytes
stop_reason`.
  Mirrors the existing `DAMACY_TRACE_READS` pattern.
- **`bench/sweep.py`**: `reads/wave`, `shards/wave`, `stop d/h/c/v`
columns.
- **`bench/scenarios/array-starve.json`**: an `n_zarrs` sweep scenario
(large
  shards) for `sweep.py --param dataset.n_zarrs`.

Always-on cost is one O(n) pass plus a small distinct-shard scan per
wave
(n_reads is post-coalesce, typically 100–250); negligible vs io/decode.
The trace
is opt-in.

## What it found (full writeup on #154)

Using these counters, io throughput scales cleanly with `shards/wave` —
6→1.09,
37→2.37, 236→4.32 GB/s — confirming a real read-width effect on
few-shard sets.
But it's mild: damacy stays io-bound and **beats tensorstore on every
measured
dataset, including a 1-array one**. The catastrophic losses that
motivated #154
were decode amplification, not array count. So #154 wants re-scoping to
"io-pool under-utilization on few-shard datasets" rather than a
competitiveness bug.

## Not included

`bench/tensorstore_bench.py` (used for the comparison) lives on its own
branch
`bench-tensorstore-comparison` (#153) and is intentionally not
duplicated here.

ctest 33/33 on L40.

Co-authored-by: Nathan Clack <nclack@biohub.org> 7d860ac
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