Repository navigation
Fan out compute-bound row-scan kernels from 128k rows - #424
FarhanAliRaza merged 5 commits into
Conversation
Every row-scan kernel shared PAR_THRESHOLD = 1<<19, introduced for bin_2d/histogram and sweep-applied to the rest in 84ff460 (calibrated on a 4-core sandbox at 10M rows; the 131k-512k band never measured). M4 and uniform histogram run ~2.5-3 ns/row serially, so a scoped 18-way fan-out (~0.25-0.3 ms spawn+join) pays for itself from ~128k rows - the crossover zone maps already use (two 65,536-row chunks). Split the gate per cost class: par_compute_threads (128k) for m4_indices and histogram_uniform; par_threads keeps the general 512k gate for everything else. Routing stays bitwise-deterministic - thresholds choose segment count only, covered by the existing thread-count parity fuzz. Measured on an i7-12800H (20 threads), interleaved medians: m4 300k 0.82-0.98 -> 0.31-0.47 ms, m4 500k 1.34-1.59 -> 0.33-0.54 ms; histogram similar (2-4x across the band); 8x500k-line dashboard build_payload 11.1-13.8 -> 3.5-5.2 ms. Payloads byte-identical to main (spec+blob SHA-256) for decimated line, bar, and direct scatter. Encode/normalize re-gating was tried and reverted: normalize is ~1.2 ns/row (regresses 2-4x at a 2M gate) and encode's crossover is machine-state-dependent, so both keep the 512k gate.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Threshold routing src/kernels.rs |
Worker selection accepts an explicit threshold. M4 and uniform histograms use the 128k compute threshold; general scans retain the 512k threshold. Histogram workers are capped by points per bin. |
M4 bucket partitioning src/kernels.rs |
M4 worker count is capped by bucket count. Binary search replaces row-by-row scanning to find bucket boundaries. |
Validation and threshold guidance src/kernels.rs, spec/benchmarks/results.md, spec/design/rust-engine.md, news/424.performance.md |
Tests cover threshold routing, histogram output parity, and M4 boundary-search work. Documentation describes thresholds, worker caps, and M4 partitioning. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Refactor
Possibly related PRs
- reflex-dev/xy#535: Changes the M4 decimation path through bucket scanning and SIMD argmin/argmax; this PR changes M4 worker routing and bucket-boundary search.
Suggested reviewers: alek99, farhanaliraza
Merge Risk: 
b6a3b
-
No concrete issue remains that calls for a change before merging, subject to normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 1 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the main change: lowering fan-out for compute-bound row-scan kernels to 128k rows. It matches the implementation and PR objectives. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 1 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/kernels.rs (1)
6881-6892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd parity coverage for the newly reachable kernel paths.
The added test calls only
par_threads_above_for. It does not executem4_indicesorhistogram_uniformatPAR_THRESHOLD_COMPUTE - 1andPAR_THRESHOLD_COMPUTE. Add serial-versus-dispatched comparisons for both kernels, including exact histogram output equality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernels.rs` around lines 6881 - 6892, Extend par_thread_gates_route_by_cost_class to invoke both m4_indices and histogram_uniform at PAR_THRESHOLD_COMPUTE - 1 and PAR_THRESHOLD_COMPUTE, comparing serial and dispatched results. For histogram_uniform, assert exact equality of the complete histogram outputs while preserving the existing threshold and core-count coverage.spec/benchmarks/results.md (1)
742-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMention the grid-aware
bin_2dgate.
bin_2duses the 512k row threshold as a base gate, but it also applies a points-per-cell rule.src/kernels.rs, Line 6868, keepsbin_2dserial above that threshold when the grid is large. State that the 512k value is not an unconditional fan-out threshold.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/benchmarks/results.md` around lines 742 - 745, Update the benchmark description near the per-cost-class gates to explicitly identify the grid-aware bin_2d gate: 512k rows is its base threshold, but parallel fan-out also depends on the points-per-cell rule, with large grids remaining serial above that threshold as implemented by bin_2d. Clarify that 512k is not an unconditional fan-out threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/kernels.rs`:
- Around line 4606-4612: Update the worker-count selection passed to
histogram_uniform_impl so parallel fan-out accounts for both data.len() and
out.len(), limiting workers when the bin count makes per-worker scratch
allocation or merge cost excessive. Preserve the existing serial behavior for
large-bin histograms, and add a regression test covering an out.len() value in
the 128k–512k range.
---
Nitpick comments:
In `@spec/benchmarks/results.md`:
- Around line 742-745: Update the benchmark description near the per-cost-class
gates to explicitly identify the grid-aware bin_2d gate: 512k rows is its base
threshold, but parallel fan-out also depends on the points-per-cell rule, with
large grids remaining serial above that threshold as implemented by bin_2d.
Clarify that 512k is not an unconditional fan-out threshold.
In `@src/kernels.rs`:
- Around line 6881-6892: Extend par_thread_gates_route_by_cost_class to invoke
both m4_indices and histogram_uniform at PAR_THRESHOLD_COMPUTE - 1 and
PAR_THRESHOLD_COMPUTE, comparing serial and dispatched results. For
histogram_uniform, assert exact equality of the complete histogram outputs while
preserving the existing threshold and core-count coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f850d5c1-353f-476d-bd09-7632937cd236
📒 Files selected for processing (3)
spec/benchmarks/results.mdspec/design/rust-engine.mdsrc/kernels.rs
Merging this PR will not alter performance1 warning, 1 note
Comparing 
Footnotes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The kernel-parallelization note listed bin_2d among the general scans that "fan out from 512k", which reads as an unconditional threshold. It is a base gate: bin_2d_threads also tracks points per cell and keeps a large grid serial above 512k, because the per-thread grids and their merge dwarf the scan (asserted in kernels.rs: bin_2d_threads(1 << 20, 1 << 20) == 1). Raised in CodeRabbit review of reflex-dev#424: reflex-dev#424 (review)
histogram_uniform_impl gives every worker its own vec![0u64; n_bins] and merges at O(threads * n_bins). Neither term shrinks with the row count, so gating on data.len() alone under-counts the cost of fanning out: with enough bins the merge outweighs the scan it parallelizes. Base had this too -- its 512k row gate ignored out.len() as well -- but lowering the compute gate to 128k amortizes the same merge over ~4x fewer rows, moving the crossover where fan-out stops paying down with it. Cap workers at points per bin, mirroring bin_2d_threads, whose per-thread grids are this kernel's per-thread bins. A bin vector as large as the input now stays serial; scratch is bounded at threads * n_bins <= n counters. Ordinary bin counts are unaffected: at 128k rows and the 512 bins the benchmarks use, the ratio is 256 and clamps to the same 18-worker cap, so the histogram numbers measured for this PR stand. Raised by both review bots on reflex-dev#424: reflex-dev#424 (comment) reflex-dev#424 (comment)
|
Both bots caught the same thing. Fixed in c11d71b by capping workers at points per bin, mirroring (n / n_bins.max(1)).clamp(1, par_threads_above(n, PAR_THRESHOLD_COMPUTE))Normal bin counts are untouched. At 128k rows and the benchmarks' 512 bins the ratio clamps to the same 18-worker cap, so the PR's numbers stand. Tests cover routing and 128k-512k parity; specs updated. Nitpicks: |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
FarhanAliRaza
left a comment
FarhanAliRaza
left a comment
There was a problem hiding this comment.
Changes requested. The main speedup reproduces and output is unchanged. Three items block the merge:
- M4 regresses on clustered x in the 128k–512k band. It runs 0.37× at 200k rows and 0.42× at 300k rows when 97% of rows fall in one bucket. The split search scans linearly. Use a binary search for each split. Details and numbers are in the inline comment on
src/kernels.rs:2505. - The news fragment is missing. The PR changes
src/, andmainnow has a changelog check that requires a fragment (.github/workflows/changelog.yml). The PR branch predates that check, so CI was green, but it will fail once the branch takes inmain. Addnews/424.performance.md, for example withmake news NAME=424.performance.md. - Spec drift: the M4
n_bucketsworker cap is not in E5 or inresults.md. See the inline comment onspec/design/rust-engine.md.
What I checked (base eb37670 vs head 226afcf, release build, 4-core Linux):
- Speedup reproduces, scaled down to 4 cores. ctypes kernel bench, interleaved, median of 21 runs:
- M4 at 200k/300k/500k rows: 1.43× / 1.49× / 2.50×.
- Histogram with 512 bins at 200k/300k/500k rows: 1.96× / 1.76× / 2.62×.
- M4 at 100k rows: 0.97×, which is noise, as expected below the gate.
- The repo harness agrees.
scripts/bench.py --sizes 5e5,3e5,2e5: M4 went from about 250–300 to 420–580 Mpt/s at 200k–300k rows. Zone maps andencode_f32did not move beyond noise. The first fan-out in a process carries a one-time thread start-up cost, which also shows on base at 512k rows and above. - Output does not change. I built a dashboard example with lines at 150k/300k/450k/500k rows, a 300k clustered "burst" line, and histograms at 300k rows/512 bins and 200k rows/4096 bins. Its
to_htmloutput is byte-identical between base and head. Rendered in headless Chromium, every chart draws, the screenshots are byte-identical, and the console shows no errors or warnings. cargo test --releasepasses (141 tests). That includes the new routing tests, the high-bin-count histogram parity test, and the existing parity fuzz.- The PR merges cleanly into current
main. - The high-bin-count histogram worry from the earlier bot threads is now handled by
histogram_threads. From Python it cannot be reached anyway, because_native.histogram_uniformcaps bins at 4096.
Generated by Claude Code
 [Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryM4 and uniform histograms can now use multiple workers from 131,072 rows. Other row scans keep the 524,288-row threshold.
Reviews (1) · Last reviewed commit: "Fix clustered M4 fan-out in compute band" · Reviewed by Greptile |
Fan out compute-bound row-scan kernels from 128k rows
Summary
Every row-scan kernel in
src/kernels.rsshared one fan-out gate,PAR_THRESHOLD = 1 << 19(524,288 rows). It was introduced forbin_2d/histogram, then sweep-applied to the rest from a 4-core calibration at 10M rows. The 128k–512k band was never measured, and it is where multi-chart dashboards live.M4 and uniform histogram run ~2.5–3 ns/row serially, so an 18-way fan-out repays its ~0.25–0.3 ms spawn+join well before 512k; the shared gate strands them on one core, costing 2.5–3.6×. This gives them their own gate,
PAR_THRESHOLD_COMPUTE = 1 << 17(131,072 rows, the crossover zone maps already use). Every other kernel keeps 512k.Simply lowering the single gate was measured and rejected:
encode_f32at 200k rows is ~5.7× slower fanned out, since spawn+join dominates a ~0.2 ns/row memory-bound pass. Hence per cost class.Changes
src/kernels.rs: add the compute-class gate and generalize the routing helpers so the threshold is a parameter rather than baked into the lookup. The CodSpeed-serial check and the 18-worker cap stay in the shared helper:The two compute-bound call sites pass the compute constant inline, matching how
raster_fanoutalready takes its per-workload threshold as an argument:Plus
par_thread_gates_route_by_cost_class, a deterministic routing test across 1/4/20/64 cores: the compute gate fans out at exactly its threshold, and a compute-sized input still stays serial under the general gate. That is the invariant distinguishing the two classes.spec/design/rust-engine.md: E5 updated to document per-cost-class gates with their measured ns/row and break-evens.spec/benchmarks/results.md: the kernel-parallelization note now states the 128k compute gate alongside the 512k general gate.Performance
i7-12800H (20 threads), Linux 6.8, fat-LTO release build. Interleaved base/branch rounds; medians of 15 runs after 3 warmups, ms:
build_payloadbuild_payloadKernels left on the general gate are unchanged within noise (
encode_f32500k 0.124 → 0.132,min_max500k 0.492 → 0.498,is_sorted500k 0.247 → 0.257), confirming the re-gate is confined to the two compute-bound kernels. Below 128k rows the code path is identical to before, so small-N behavior is untouched by construction.Testing
cargo test --release→ green, including the new routing test and the existing parity fuzz (serial and fanned-out results bitwise identical).cargo clippy --all-targets,cargo fmt --check,ruff check,ruff format --check→ clean.ty check→ unchanged from base; no Python touched.pytest→ green apart from two failures that reproduce on an unmodified base checkout (packaging-layout drift, unrelated).Risks & rollout
ABI_VERSIONbump and no Python or JS changes.Summary by CodeRabbit
Performance Improvements
Documentation