Spill reduce/read profiling split — #280 step 0 (measurement enablement)#285
Spill reduce/read profiling split — #280 step 0 (measurement enablement)#285espg wants to merge 2 commits into
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Reviewed the reduce/read profiling split against #280 step 0's stated intent and the repo conventions. The change is small (+24 lines), correct, and lands the measurement it claims. Verified in detail:
Timing-split correctness — clean. In _fold_block, t1 is taken immediately after spill_read_s is charged, and spill_reduce_s is charged at the end of the per-partition loop. The reduce window therefore covers exactly _group_columns + the build/merge loop, sequential after read_partition — read-wall and reduce-wall are disjoint, with no double-counting and no path folded into the wrong bucket. spill_read_s semantics are unchanged (the group/build compute was previously untimed, not part of spill_read_s), so this is purely additive instrumentation.
Thread-safety — fine. spill_reduce_s is written only inside _fold_block, which runs either on the overlap reducer thread or on the main thread (final still-open block in _chunk_outputs_merged), never concurrently — _join_reducer precedes both the next close and the final fold. The += is GIL-safe with a single writer, and worker.py reads the value only after the aggregate phase completes.
Downstream threading — consistent everywhere. spill_reduce_s is a _s-suffixed float, so it flows generically through worker.py phase_timings, telemetry's seconds-vs-_bytes split (telemetry.py:161), the per-key merge sum (telemetry.py:234), and schema.py's dict[str, float]. No hardcoded keyset anywhere drops it; the exact-set assertion in test_profile_carries_spill_instrumentation was updated to include it, and since worker.py writes the key unconditionally under spill_mode, single-block runs still carry it (value 0.0).
Conventions — met. spill.py is ~795 lines (well under the ~1000 limit), no new dependency, terse and well-commented, naming mirrors spill_read_s/spill_write_s, and a new multi-block test covers the populated path.
One diff-scoped suggestion (non-blocking), inline: the single-block profile test asserts spill_reduce_s >= 0 where it is deterministically 0.0; tightening to == 0.0 would pin the single-block-no-fold half of the split. See the inline comment.
Two awareness notes (out of scope for this PR, for @espg):
(1) spill_reduce_s measures only the multi-block fold's reduce-CPU; the single-block pooled reduce (in the untimed _chunk_outputs_exact machinery) is not captured. This matches the stated #280 target (the 158-granule CONUS slab is multi-block), so it is correct for the measurement's purpose — flagging only so the profile is read as "multi-block fold reduce", not "all reduce".
(2) Under overlap=True, _fold_block runs on the reducer thread concurrently with main-thread network reads, so spill_reduce_s + spill_read_s overlap the read phase in wall-clock and are not additive with total wall. The ratio reduce:read within the fold (what #280 step 0 needs) stays valid since the two are sequential inside _fold_block. This is pre-existing behavior for spill_read_s, not introduced here.
No blocking issues. The split is real and the profile prerequisite is sound.
Generated by Claude Code
Refs #280. Step 0: before choosing how to parallelize the spill reducer, the profile has to show whether the fold is reduce-CPU-bound or read-I/O-bound (post-#279 the reduce is vectorized numpy, so this may have flipped).
What
Adds
spill_reduce_sto the spill profile — wall spent in the block fold's compute (_group_columns+build_tdigest+ merge), split from the existingspill_read_s(read-back I/O). Populated in the multi-block regime (_fold_block); the single-block exact path has no fold.Why this gates #280
The A/B/pipelining decision hinges on the split (measured on the 158-granule CONUS slab, not 88S):
spill_reduce_s≪spill_read_s→ compute parallelism is pointless; the lever is I/O (output-preserving prefetch pipelining).spill_reduce_sis significant → coupled parallel reduce (reduce_workers=P⇄block_bytes/P, since one partition reduce already peaks at ~60% mem) earns its keep.Also relevant: one partition's reduce peak is ~0.6×mem (
_BUILD_MULT=3,partition_bytes=0.2×mem), so partition-parallelism is not free/scheduling-only — it requires shrinking blocks, which changes (in-tolerance) digest bytes. That's why we measure before building.Tested
test_multi_block_reduce_time_is_captured(reduce_s > 0 on a forced-multi-block run) + updatedtest_profile_carries_spill_instrumentationkey set.pytest tests/test_spill.py→ 50 passed; ruff clean.Next (pending the measurement)
Run the 158g CONUS slab with this and read the reduce/read split → pick coupled-parallel vs pipelining → implement Flavor A. Not byte-affecting; safe to land as the measurement prerequisite.