Repository navigation
Reduce monomorphization in sequence and run-end take - #11102
Conversation
Merging this PR will improve performance by 12.07%1 warning
Performance Changes
Tip Curious why performance improved? Comment Comparing Footnotes
|
Short walltime benchmarks can report changes on unchanged code because CodSpeed compares the fastest sample. Batch the bitpacked filter and take samples, and keep unreliable cases local. Exclusions live beside the benchmark definitions, using `#[cpu_features(exclude = ["neon"])]` for whole CPU variants and source-level selection for individual inputs. CI commands stay generic. Move the chunked-take family to walltime with 16 iterations per sample so it measures the operation's allocator and syscall costs. Keep its x86 variants local after repeatability checks failed; all 27 NEON cases remain in CI. The guide retains practical examples and measurement guidance without an exclusion catalog or a repeatability tool. <details> <summary>Measurement evidence</summary> Spread is `max / min - 1` across independent processes, with 1000 Divan samples per process and ten processes on each of three matching hosts. | Case | Before | Fixed sampling | | --- | --- | --- | | AVX2 `filter[80]`, sample size 64 | 9.36% | 1.45% | | AVX-512 `threshold[80]`, sample size 16 | 8.26% | 1.69% | Across independent base and head AVX2 builds, the fixed filter's worst spread was 4.66% over 60 processes. AVX-512 filter cases stayed within 1.78%. The excluded NEON string case still varied by 6.38%. Measurements used existing CI executables with matching sample-size overrides from [run 41449104113](https://github.com/vortex-data/vortex/actions/runs/41449104113) (`44bdefff6e69`) and [run 41442182591](https://github.com/vortex-data/vortex/actions/runs/41442182591) (`7c9bb58e39fe`), on three `c7i.metal-24xl` hosts for x86 and three `c7g.metal` hosts for NEON. These measure repeatability, not kernel speedups. Other Rust exclusions follow unexplained variation recorded in [#10808](#10808) and repeated on the workflow-only [#11095](#11095 (comment)). The two Python integer scalar scans include variable calling-thread reactor polling, visible in [#11102](#11102), even with memory mapping and background workers disabled. The chunked-take warning on `dfb32c5`, compared with `d8e7467`, came from mimalloc lazily allocating page-map metadata. The affected `small_m/shuffled/primitive/nonnull/chunks=2048/indices=16` case made one `mmap` and one `prctl` call totaling 23.3 µs; the base trace made neither. An untimed warm-up removed the syscall warning but produced eight regression alerts with unchanged kernel code under allocation-excluded simulation. Walltime measures elapsed time without that allocation exclusion and establishes separate per-CPU baselines. Walltime repeats used the CI executables from `29920deef0`, 1000 samples per process, and ten independent processes on each of three matching hosts per architecture. All 27 NEON cases stayed within 4.35% spread; the originally flagged case stayed within 2.88%. The worst x86 spreads were 11.04% for AVX2 and 11.20% for AVX-512. Increasing the sample size to 64 on two troublesome x86 cases still produced 12.15% and 13.38% spread, so those CPU variants remain local through the macro exclusion. </details> Focused native and simulation Clippy checks passed. Test-mode runs confirmed the local, AVX2, and NEON string case selection and the macro's NEON exclusion. Pinned-nightly formatting, Ruff, ty, YAML linting, and patch whitespace checks passed. All 66 CI checks passed on `29920deef0`, including CodSpeed analysis with zero syscall warnings. The final x86 exclusion passed focused native and simulation Clippy checks and test-mode checks for both excluded variants. CI is rerunning for that final change. Local Git hooks were bypassed. AI assistance: Codex helped investigate, implement and prepare this PR. --------- Signed-off-by: Connor Tsui <[email protected]>
Read take indices through their unsigned reinterpretation, matching the varbin and list take kernels, so signed index types no longer produce separate copies of the kernels. The sequence take kernel also evaluates in the unsigned counterpart of the sequence ptype, since wrapping arithmetic produces the same bits in either signedness, and reinterprets the result back. This cuts it from 64 instantiations to 16. The run-end take kernel drops from 8 to 4 index widths in TakeExecute. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <[email protected]>
take_inner had grown to eight arguments, tripping clippy's too_many_arguments lint. It now derives base, multiplier, ptype and length from the sequence view itself. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <[email protected]>
Negative indices of every signed width are rejected as out of bounds, and a negative value under a null index is ignored. The take tests used an empty session, so the Sequence take kernel was never registered and every case, including the conformance suite, ran through the canonical primitive take. Initialize the session so the tests exercise the Sequence kernel. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <[email protected]>
Draws ascending and descending sequences of every integer type with negative and positive bases, takes with indices of every integer type, and compares against an exact i128 evaluation of base + index * multiplier. Null indices hold negative or out-of-bounds values that the kernel must ignore. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <[email protected]>
1eb9b44 to
7815d13
Compare
Trim the doc comment of take_inner. Test negative indices with one index type, because the check does not depend on the type. Signed-off-by: Robert Kruszewski <[email protected]>
Replace the exact i128 model with a check against a take from the canonical values of a generated sequence. Signed-off-by: Robert Kruszewski <[email protected]>
Draw the base, multiplier and length freely and reject sequences that do not fit the type, so that the test states only the property. Signed-off-by: Robert Kruszewski <[email protected]>
| /// `T` and `O` are unsigned to keep 4x4 instantiations, because wrapping arithmetic produces the | ||
| /// same bits in either signedness. | ||
| fn take_inner<T: UnsignedPType, O: SequenceValue + UnsignedPType>( |
There was a problem hiding this comment.
This is still pretty crazy to me...
Do we want to more formally wrap this logic somehow? Like some macro or function or trait that says "we can do arithmetic on this in both signed and unsigned"?
Not a blocker.
There was a problem hiding this comment.
This is just how 2's complement math behaves
Read take indices through their unsigned reinterpretation, matching the varbin and list take kernels, so signed index types no longer produce separate copies of the kernels.
The sequence take kernel also evaluates in the unsigned counterpart of the sequence ptype, since wrapping arithmetic produces the same bits in either signedness, and reinterprets the result back. This cuts it from 64 instantiations to 16. The run-end take kernel drops from 8 to 4 index widths in TakeExecute.