Fan out compute-bound row-scan kernels from 128k rows - #424
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. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe kernel parallelism logic now separates compute-bound and general scan thresholds. M4 decimation and uniform histograms use the 128k threshold. Histogram worker counts also respect points-per-bin limits. Tests and documentation cover the updated behavior. ChangesKernel parallelism thresholds
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 performance
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.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/kernels.rs">
<violation number="1" location="src/kernels.rs:4610">
P2: Lowering the fan-out gate for histogram_uniform to 128k rows (PAR_THRESHOLD_COMPUTE) only accounts for row count, not bin count. Since histogram_uniform_impl allocates one full bin vector per worker and merges with O(threads * n_bins) cost, a call with a large `out.len()` in the newly-lowered 128k–512k row range can now spawn up to MAX_ROW_THREADS per-worker bin allocations that were previously avoided, risking excessive memory use or a merge cost that outweighs the parallel speedup. Consider factoring bin count into the thread-count decision (e.g., a combined row*bin scratch-budget check) and add a regression test for large `out.len()` in this range.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| data, | ||
| lo, | ||
| hi, | ||
| par_threads_above(data.len(), PAR_THRESHOLD_COMPUTE), |
There was a problem hiding this comment.
P2: Lowering the fan-out gate for histogram_uniform to 128k rows (PAR_THRESHOLD_COMPUTE) only accounts for row count, not bin count. Since histogram_uniform_impl allocates one full bin vector per worker and merges with O(threads * n_bins) cost, a call with a large out.len() in the newly-lowered 128k–512k row range can now spawn up to MAX_ROW_THREADS per-worker bin allocations that were previously avoided, risking excessive memory use or a merge cost that outweighs the parallel speedup. Consider factoring bin count into the thread-count decision (e.g., a combined row*bin scratch-budget check) and add a regression test for large out.len() in this range.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/kernels.rs, line 4610:
<comment>Lowering the fan-out gate for histogram_uniform to 128k rows (PAR_THRESHOLD_COMPUTE) only accounts for row count, not bin count. Since histogram_uniform_impl allocates one full bin vector per worker and merges with O(threads * n_bins) cost, a call with a large `out.len()` in the newly-lowered 128k–512k row range can now spawn up to MAX_ROW_THREADS per-worker bin allocations that were previously avoided, risking excessive memory use or a merge cost that outweighs the parallel speedup. Consider factoring bin count into the thread-count decision (e.g., a combined row*bin scratch-budget check) and add a regression test for large `out.len()` in this range.</comment>
<file context>
@@ -4596,7 +4603,13 @@ pub fn stratified_sample_mask<T: Copy + Sync + Into<u64>>(
+ data,
+ lo,
+ hi,
+ par_threads_above(data.len(), PAR_THRESHOLD_COMPUTE),
+ out,
+ )
</file context>
There was a problem hiding this comment.
Should be addrsssedin recent commits
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.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/kernels.rs">
<violation number="1" location="src/kernels.rs:2505">
P2: M4 calls with `n_buckets == 1` now enter the parallel splitter from 128k rows even though only one segment can exist. Each candidate boundary is linearly advanced to `end`, adding roughly `threads / 2` extra full scans; capping workers by `n_buckets` avoids this regression.</violation>
</file>
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.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/kernels.rs">
<violation number="1" location="src/kernels.rs:2505">
P2: Clustered or duplicate-x M4 windows in the new 128k–512k fan-out band can regress badly: every proposed split scans through the same giant bucket, then only one segment is executed. Consider finding bucket boundaries with a single pass/binary search, or falling back before paying these repeated scans.</violation>
<violation number="2" location="src/kernels.rs:2505">
P3: The per-cost-class routing docs updated in this PR enumerate the worker caps for `bin_2d` (points per cell) and `histogram` (points per bin), but the newly added `m4` cap at `n_buckets` is not documented. Since m4's parallel path only ever splits at bucket boundaries (at most `n_buckets` distinct segments are possible), capping workers at `n_buckets` is a benign optimization — but it is freshly introduced routing behavior. Per the repo's spec-tracking contract (AGENTS.md: "Every decimation/tier decision is recorded in the spec, never silent"), it would be worth one sentence alongside the existing accumulator-cap note in `spec/design/rust-engine.md` (E5) and `spec/benchmarks/results.md` so the documented fan-out contract matches the implementation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| start, | ||
| end, | ||
| par_threads(end - start), | ||
| par_threads_above(end - start, PAR_THRESHOLD_COMPUTE).min(n_buckets), |
There was a problem hiding this comment.
P2: Clustered or duplicate-x M4 windows in the new 128k–512k fan-out band can regress badly: every proposed split scans through the same giant bucket, then only one segment is executed. Consider finding bucket boundaries with a single pass/binary search, or falling back before paying these repeated scans.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/kernels.rs, line 2505:
<comment>Clustered or duplicate-x M4 windows in the new 128k–512k fan-out band can regress badly: every proposed split scans through the same giant bucket, then only one segment is executed. Consider finding bucket boundaries with a single pass/binary search, or falling back before paying these repeated scans.</comment>
<file context>
@@ -2502,7 +2502,7 @@ pub fn m4_indices(x: &[f64], y: &[f64], x0: f64, x1: f64, n_buckets: usize) -> V
start,
end,
- par_threads(end - start),
+ par_threads_above(end - start, PAR_THRESHOLD_COMPUTE).min(n_buckets),
)
}
</file context>
| start, | ||
| end, | ||
| par_threads(end - start), | ||
| par_threads_above(end - start, PAR_THRESHOLD_COMPUTE).min(n_buckets), |
There was a problem hiding this comment.
P3: The per-cost-class routing docs updated in this PR enumerate the worker caps for bin_2d (points per cell) and histogram (points per bin), but the newly added m4 cap at n_buckets is not documented. Since m4's parallel path only ever splits at bucket boundaries (at most n_buckets distinct segments are possible), capping workers at n_buckets is a benign optimization — but it is freshly introduced routing behavior. Per the repo's spec-tracking contract (AGENTS.md: "Every decimation/tier decision is recorded in the spec, never silent"), it would be worth one sentence alongside the existing accumulator-cap note in spec/design/rust-engine.md (E5) and spec/benchmarks/results.md so the documented fan-out contract matches the implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/kernels.rs, line 2505:
<comment>The per-cost-class routing docs updated in this PR enumerate the worker caps for `bin_2d` (points per cell) and `histogram` (points per bin), but the newly added `m4` cap at `n_buckets` is not documented. Since m4's parallel path only ever splits at bucket boundaries (at most `n_buckets` distinct segments are possible), capping workers at `n_buckets` is a benign optimization — but it is freshly introduced routing behavior. Per the repo's spec-tracking contract (AGENTS.md: "Every decimation/tier decision is recorded in the spec, never silent"), it would be worth one sentence alongside the existing accumulator-cap note in `spec/design/rust-engine.md` (E5) and `spec/benchmarks/results.md` so the documented fan-out contract matches the implementation.</comment>
<file context>
@@ -2502,7 +2502,7 @@ pub fn m4_indices(x: &[f64], y: &[f64], x0: f64, x1: f64, n_buckets: usize) -> V
start,
end,
- par_threads(end - start),
+ par_threads_above(end - start, PAR_THRESHOLD_COMPUTE).min(n_buckets),
)
}
</file context>
1 participant
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