Scan selection candidates in place (ABI 42 -> 43) - #323
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a native row-restricted range-selection kernel, exposes it through Python bindings, updates ChangesRow-restricted range selection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant select_range
participant PythonBinding
participant xy_range_indices_rows
participant RustKernel
select_range->>PythonBinding: provide coordinates and candidate rows
PythonBinding->>xy_range_indices_rows: call native row-filtering API
xy_range_indices_rows->>RustKernel: validate and scan candidate rows
RustKernel-->>xy_range_indices_rows: return matching canonical IDs
xy_range_indices_rows-->>PythonBinding: return filtered IDs
PythonBinding-->>select_range: provide selected rows
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will improve performance by ×2
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_select_box_zone_pruned_1m |
3.4 ms | 1.7 ms | ×2 |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing alek/mem-audit-2-abi (c24f1b2) with main (7c377ce)
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
0c49eca to
d965032
Compare
Alek99
changed the title
4cf2074 to
5c51d09
Compare
`select_range` prunes by zone map before it scans, and then threw the saving away: the pruned branch gathered `x[candidates]` and `y[candidates]` into two fresh f64 columns purely so it could call `range_indices`, which only knows how to walk a whole column. Sixteen bytes per candidate, which produces the backwards result that selecting *part* of a trace costs far more than selecting all of it — on a 10M-row scatter, a half-domain box peaked at 134.6 MB while a full-domain box peaked at 38.1 MB. `xy_range_indices_rows` is the rectangular twin of the lasso's `xy_polygon_select`: same `(x, y, len, rows, n_rows, ..., out)` shape, same inclusive comparison chain as `range_scan_scalar`, same order-preserving gather-down across worker segments. The pruned branch now reads through the row ids and gets canonical ids straight back, so both the gathers and the `candidates[hits]` re-index are gone. The half-domain case drops to 57.6 MB and the full-domain case is untouched. ABI_VERSION goes 42 -> 43 in `src/lib.rs` and `python/xy/_native.py` together. Parity is the gate, not inspection: the new kernel is tested against `range_indices` over the gathered form for five windows (including empty, degenerate-point and whole-domain) crossed with five row subsets and five sizes up to 100k, plus a 2M-row case that exercises the threaded path, NaN and infinities on both axes, inclusive bounds, unsorted row order, invalid windows and an out-of-range row id. `test_selection_prunes_non_overlapping_zone_chunks` now also asserts that exactly one scan happens and that it reads the whole column, which is what proves the gather is gone. 118 Rust tests, 133 ABI smoke checks and the full Python suite pass; a 97-artifact output fingerprint sweep is identical to main.
`range_indices_rows` and `polygon_select` index `x[row]` and left the report to Rust's bounds check, which panics. `ffi_guard` turns a panic into the entry point's sentinel only where panics unwind, and the PyEmscripten wheel is built `-C panic=abort` precisely so they cannot: there the same id aborted the Pyodide instance (exit 134) rather than raising ValueError. `xy.kernels` is public API, so these row arrays are caller data — the documented contract has to hold on every target, not just the ones that unwind. Both kernels now read through `get` and return `Option`, with the FFI layer mapping `None` to the existing sentinel. This is free: the same two bounds checks happen either way, so only the reporting changes. A separate validating pass over `rows` was not free and is deliberately not what this does — one serial sweep of 5M u32 cost more than the entire parallel scan (1.0 ms -> 2.4 ms), where reading through `get` measures 1.01 ms against 1.02 ms, and `polygon_select` 36.3 ms against 36.4 ms.
claiming 43 for `xy_range_indices_rows`, and open PR #327 already claims 44. Both edits touch the same two lines with the same value, so such a merge is clean and nothing complains — which is the problem. One number would name two different ABIs, and `scripts/abi_smoke.py` compares the built library against the wrapper's constant, so it cannot catch a collision where both sides agree: a cdylib built from either tree would load against the other's wrapper and fail at dlsym instead of at the version check that exists to prevent exactly that. 45 is unclaimed across every open PR. It also fails loudly rather than silently if #327 lands second, because its 43 -> 44 edit then conflicts against a main at 45 instead of merging into agreement.
The out-of-range guarantee the previous commit made stops at the ctypes boundary: the wrapper reached the kernel through `ascontiguousarray(rows, dtype=np.uint32)`, an unchecked C cast. An id of `2**32 + 3` arrives as row 3 — in range, indistinguishable from a real id, so the bounds check passes and the call returns a row the caller never asked for. `kernels.range_indices_rows(x, y, [2**32 + 3], ...)` answered `[3]`; a negative id only errored by accident, because `-1` wraps to 4294967295 and that happened to exceed the column length. Both row-restricted kernels now widen and range-check before casting. The internal caller passes uint32 already (`_expand_zone_chunks`), which takes the pass-through branch, so the hot path is untouched — 5M candidates still 1.00 ms.
The range check widened through `astype(np.int64)`, which still let floats through silently and, worse, decided them differently per architecture: `[3.9]` selected row 3, and `[nan]` selected row 0 on arm64 because the NaN cast saturates, where x86_64 traps it to INT64_MIN and the range check would have caught it. A guard whose answer depends on the wheel is not a guard. The cast also emitted a RuntimeWarning from inside the wrapper. Integer dtypes only now, with an empty array exempted because `np.asarray([])` is float64 and selecting no rows is not a dtype error.
ae59224 to
c24f1b2
Compare
1 participant
Contains a C-ABI change:
ABI_VERSION42 -> 43. From the second memory auditpass (follow-up to #309); sibling PRs carry the encode paths (#320), the Python
paths (#321), the Rust core (#322), and the legend cache.
The problem: partial selections cost more than total ones
select_rangeprunes by zone map before it scans, and then threw the savingaway. The pruned branch gathered the candidate rows into fresh columns purely so
it could call
range_indices, which only knows how to walk a whole column:Two f64 gathers, 16 bytes per candidate, which produces a backwards result — on a
10M-row scatter:
Selecting half the trace cost 3.5x what selecting all of it cost.
The fix
xy_range_indices_rowsis the rectangular twin of the lasso'sxy_polygon_select, which already takes arowsargument: same(x, y, len, rows, n_rows, ..., out)shape, the same inclusive comparison chainas
range_scan_scalar, and the same order-preserving gather-down across workersegments. The pruned branch reads through the row ids and gets canonical ids
straight back, so both gathers and the
candidates[hits]re-index are gone.The half-domain case drops to 57.6 MB. The whole-domain case is untouched.
It is a CPU win too. I had hedged this as "probably, but inside the local noise
band"; CodSpeed measured it cleanly in simulation mode:
test_select_box_zone_pruned_1m102 benchmarks untouched, 0 regressions. Removing two f64 gathers over the
candidate set removes the memory traffic that dominated that path.
ABI_VERSIONis bumped 42 -> 43 insrc/lib.rsandpython/xy/_native.pytogether, per the contributor contract.
Why a new entry rather than a parameter on
xy_range_indicesThe whole-column form stays the right call when nothing is pruned (no row-id
array to build or read), and it is used on the drill path as well. Adding an
optional
rowswould make every caller pay a branch and a null check for a casehalf of them never hit.
Verification
Parity against the gathered form is the gate, not inspection.
test_range_indices_rows.py:crossed with four row subsets (every row, strided, last-row-only, empty) and
five sizes up to 100k
range_indicestest_selection_prunes_non_overlapping_zone_chunksnow also asserts that exactlyone scan happens and that it reads the whole column — which is what proves the
gather is gone rather than merely moved.
Plus a figure-level check that both pruning branches agree with a plain NumPy
predicate over four windows.
118 Rust tests, 133 ABI smoke checks, 2445 Python tests pass. A 97-artifact
output fingerprint sweep is identical to main.
Perf
Interleaved A/B against main with the native core swapped per arm: neutral, no
region more than 5% worse under drift-cancelling round ordering.
Summary by CodeRabbit