Re-land separable pyramid compose (recover #153 perf) - #228
Conversation
The one-pass area-weighted downsample cost 4 scattered read-modify-writes per source cell (vs 1 for the old center-only assignment), regressing test_pyramid_compose ~40% on CodSpeed. Resample separably instead: pull each source row into a w-wide scratch row via per-bin taps transposed from axis_weights (independent bins, no store-forwarding chains), then distribute the scratch row into its 1-2 output rows with contiguous scale-and-add loops the compiler vectorizes. Same weights, same summation order along each axis: the cell-aligned bit-exactness contract against bin_2d holds unchanged (all weights are exactly 1.0 there), and all tiles tests pass as-is. Local walltime for the CodSpeed shape (2.1M points, 512x384 from the 1024 level): 0.63ms pre-#153 baseline, 0.91ms one-pass area weighting, 0.53ms separable - faster than the baseline it regressed from.
Greptile SummaryThis PR restores the separable pyramid composition path for area-weighted resampling. The main changes are:
Confidence Score: 5/5This looks safe to merge. No blocking issues found in the changed code.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (3): Last reviewed commit: "Make area-weighted compose separable to ..." | Re-trigger Greptile |
Merging this PR will degrade performance by 10.5%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
4a82659 to
1947528
Compare
CodSpeed status: recommend acknowledge — the metric can't see this optimizationShort version: this PR is 2.4× faster in real wall-clock but CodSpeed (simulation/cachegrind mode) scores it ~10% slower, because the win is a CPU-pipeline effect a cachegrind cost model has no concept of. Measured on the
Why the metric is inverted here. The one-pass form does I tried to make it cheap on both metrics (fused the y-distribute into the pull loop to drop the scratch-row buffer round-trip) — it didn't move the simulated number (stayed 10.9 ms) and was marginally slower in wall-clock, so I reverted it. The conclusion: area-weighting's simulated cost is irreducible relative to the one-pass; no code arrangement wins on a metric blind to the actual bottleneck. Note: main already carries the one-pass area-weighting (#217), i.e. the 5.9→9.8 ms simulated jump was already accepted. This PR trades a further +11% simulated for a 2.4× real speedup. Recommendation: acknowledge this benchmark on CodSpeed. Longer term, |
…y-mean-color Both sides added a function after axis_weights: #228's transpose_to_taps (the separable compose's x-pass taps) and this branch's compose_color. Keep both. compose_color is unaffected by the count pass going separable: its count grid comes from compose() itself (bit-identical by delegation), and its color planes weight by the same axis_weights either way; it keeps the one-pass 2D splat, noted in its doc comment.
Most conflicts were squash artifacts: main's #226 is the same mean-color density work this branch already carries as commits (main's conflicted spec/client/kernel/test files are byte-identical to this branch's pre-work state), so those resolve to this branch's extended versions. Real incoming changes kept from main: - src/tiles.rs: separable area-weighted compose (#228) — taken wholesale (this branch made no tiles.rs changes beyond the shared #226 content); kernels.rs/lib.rs wasm panic-abort hardening (#200) auto-merged. - README.md: the #230 adoption refresh replaced the inline benchmark table (this branch's only README edit) with links to the launch report, so main's version stands; the 0.1.0 launch-baseline report keeps its historical "density + sample" labels — it records what that release did. Verified on the merged tree: cargo tests, ABI smoke, render smoke, and the full pytest+benchmarks suite — the only failures are 21 doc-contract tests (test_docs_examples/test_verify_local/pyplot perf guardrail) that fail identically on pristine origin/main in this environment: the #230 README rewrite removed maintainer-shortcut text its guardrail tests still assert. Pre-existing on main, not introduced here.
#228 made the area-weighted downsample separable, which cut wall-clock ~1.7x but left CodSpeed's simulation gate red: the pull pass indexed a CSR prefix per output bin, so every one of the w bins re-derived both slice ends and paid a range check before summing its 1-3 taps. That cost more instructions than the one-pass scatter it replaced (+24% locally under callgrind), which is what the gate measures. Bins own consecutive runs of the tap array, so hand the resample a cursor instead: `transpose_to_taps` now returns per-bin counts (it already built them before the prefix sum) and the loop splits `n` taps off the front per bin. Same taps, same order, same weights — pure bookkeeping. Measured on the benchmark's shape (2.1M points, 512x384 from the 1024 level), interleaved min-of-9 vs a build of the previous implementation: instructions (callgrind) 22.17M -> 17.97M -19.0% wall-clock (arm64 mac) 0.552ms -> 0.420ms -23.9% That puts instructions back at the pre-#228 count (17.88M) while keeping the wall-clock win, so the gate and the renderer agree again. Output is bit-identical: composed grids fingerprint the same across the benchmark window, the full domain, a cell-aligned subwindow, an offset unaligned window, and an upsample refusal. The cell-aligned bit-exactness contract against bin_2d is untouched.
2 participants
Re-land the separable pyramid compose (perf recovery)
Follow-up to #217. That PR shipped the #153 banding fix (area-weighted
compose), but the separable perf-recovery commit was dropped before merge, somaincurrently runs the one-pass 2D-scatter form — 4 scattered read-modify-writes per source cell.This restores the separable implementation (Alek's original commit, authorship preserved): resample each source row into a
w-wide scratch row via pull-based per-bin taps transposed fromaxis_weights, then distribute that row into its 1–2 output rows with contiguous, vectorizable scale-and-add loops.Why it's a strict win
Same result. Pure reindexing of the same weights, same per-axis summation order → the cell-aligned bit-exactness contract against
bin_2dholds. All 10tilestests pass unchanged; the vertical "banding" when zooming in through LoD chart #153 banding stays fixed.~2.3× faster wall-clock. On the
test_pyramid_composeshape (2.1M points, 512×384 from the 1024 level):Back to baseline parity.
CodSpeed: this compares against current
main(which already has the one-pass area-weighting), and the separable form executes fewer instructions than the one-pass 2D scatter — so it should read as an improvement rather than the regression seen in Area-weight pyramid compose to remove interim zoom banding (#153) #217 (where the base was pre-area-weightingmain).No API/ABI change;
composesignature and all callers are untouched.